[NFC][AsmPrinter] Refactor DbgVariable as a std::variant

[scott.linder]

Summary

Only a subset of the fields of DbgVariable are meaningful at any time,
and some fields are re-used for multiple purposes (for example
FrameIndexExprs is used with a throw-away frame-index of 0 to hold a
single DIExpression without needing to add another member). The exact
invariants must be reverse-engineered by inspecting the actual use of
the class, its imprecise/outdated doc-comment, and some asserts.

Refactor DbgVariable into a sum type by inheriting from std::variant.
This makes the active fields for any given state explicit and removes
the need to re-use fields in disparate contexts. As a bonus, it seems to
reduce the size on my x86_64 linux box from 144 bytes to 96 bytes.

There is some potential cost to std::get as it must check the active
alternative even when context or an assert obviates it. To try to help
ensure the compiler can optimize out the checks the patch also adds a
helper get method which uses the noexcept std::get_if.

Some of the extra cost would also be avoided more cleanly with a
refactor that exposes the alternative types in the public interface,
which will come in another patch.

[scott.linder]

I've had this patch series sitting around for a little bit, and when I rebased I noted that there was now a comment from @fdeazeve explicitly pointing out that something like this refactoring would be nice, so I finally got around to cleaning it up and pushing it. Let me know what you think!

I broke things up to give a few different options for where to "stop", in case there was aversion to changing the interface fundamentally. I think the final result of the series is both easier to read and has potential performance benefits, but I'd love to hear feedback on either part.

[fdeazeve]

Thank you so much for doing this! I was about to start working on this, so I'm thankful you posted this when you did! I'll have a look now

[fdeazeve]

Everything looks good to me, this is much cleaner! On my first encounter with this class, it took me a long time to decipher what was going on, and the variant design is way more expressive.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

149

Not for this patch, but it really bothers me that this is mutable. The original rationale must have been to allow sorting when we call the "get" method, but still...

[scott.linder]

In D158675#4617747, @fdeazeve wrote:

Everything looks good to me, this is much cleaner! On my first encounter with this class, it took me a long time to decipher what was going on, and the variant design is way more expressive.

I know exactly what you mean! I originally started the change as just a way to prove my own understanding to myself. Without having done it I would never have understood the code to begin with.

I'm glad to hear that was a more universal reaction. I think the class slowly accreted many roles/paths, and was sort of "boiled alive". The fact that the only reasonable remedy before std::variant was inheritance or a lot of strife related to using a union member for non-POD types probably didn't help either.

Edit: Also, thank you for taking a look so quickly!

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

149

It is almost spooky how you've pointed out the same things I toyed with while working on the patches originally!

I had a patch in the middle somewhere which just exposed the need to sort in the interface, but scrapped it early on. I think your set idea makes a lot of sense and I'll follow up with a patch for that as well.

[fdeazeve]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

149

One thing to consider here: sorting (either in the current vector approach or through the internal impl of std::set) require calling Expr->getFragmentInfo()->OffsetInBits as many time as comparison operations are performed, and this is possibly done multiple times for the same expression without any caching.

Something to keep in mind as a possible improvement as well (perhaps computing & storing the offset in advance)

[fdeazeve]

Hey Scott, do you think it will be possible to merge these patches soon-ish? I would like to make some further changes to the entry value side of things, and it would be nice to get this in before.

[scott.linder]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

149

I tried to do a bit of (non-statistically-rigorous) benchmarking of a few approaches.

The versions are:

• main: unmodified main branch
• patched+uncached: most straightforward switch to std::set, without any caching of the fragment info
• patched+eager_cached: above, but assume the OffsetInBits will be needed, and cache it in the constructor of FrameIndexExpr
• patched+lazy_cached: above, but re-introduce a mutable member so the comparison operator can cache the OffsetInBits on first use

The input is a precodegen dump of llvm-tblgen from a FullLTO build of main. Not sure if this is really representative, but it at least contains multi-fragment MMI entries.

If there is a better way to do the benchmark I am happy to redo it, this is just the best I could come up with that would take a reasonable amount of time.

llc -O3 build-lto/bin/llvm-tblgen.0.5.precodegen.bc -o /dev/null -filetype=obj -time-passes

 ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---

main:
22.3635 ( 28.8%)  14.2489 ( 71.7%)  36.6124 ( 37.5%)  36.6142 ( 37.5%)  X86 Assembly Printer
21.9819 ( 28.1%)  14.6388 ( 71.8%)  36.6207 ( 37.2%)  36.6238 ( 37.2%)  X86 Assembly Printer

patched+uncached:
21.3484 ( 28.0%)  14.4208 ( 72.4%)  35.7693 ( 37.2%)  35.7706 ( 37.2%)  X86 Assembly Printer
21.4563 ( 28.2%)  14.4606 ( 72.2%)  35.9169 ( 37.4%)  35.9191 ( 37.4%)  X86 Assembly Printer

patched+eager_cached:
21.2332 ( 32.0%)  14.3204 ( 76.2%)  35.5536 ( 41.8%)  35.5555 ( 41.8%)  X86 Assembly Printer
21.6691 ( 31.7%)  14.5448 ( 74.4%)  36.2139 ( 41.2%)  36.2149 ( 41.2%)  X86 Assembly Printer

patched+lazy_cached:
22.4542 ( 27.9%)  14.9983 ( 71.0%)  37.4525 ( 36.9%)  37.4537 ( 36.9%)  X86 Assembly Printer
22.9115 ( 28.3%)  14.7169 ( 69.3%)  37.6284 ( 36.8%)  37.6299 ( 36.8%)  X86 Assembly Printer

I only ran 2 trials so there is a lot of room for variance, but it seems like everything other than patched+eager_cached is more-or-less comparable when comparing User Time as a percent of compilation time. This makes some sense to me, as I suspect the typical case is to not have multiple fragments anyway, so we would never need to compare the offsets, turning the eager caching into pure overhead.

I think for the sake of simplicity in the code it is not worth having the mutable cache at all. If you agree I can post a patch! If this doesn't seem convincing I can do a more rigorous comparison.

[fdeazeve]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

149

Oh wow, thanks for doing all of this! Indeed, I agree with you that the complexity is not worth it based on these numbers. I'd say we can start with the simplest implementation and work from there if this becomes a bottleneck.

By the way, we don't need to do this as part of the existing patch series, it seems reasonable to merge the refactors first!

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

if (auto *DVal = DV.getValueLoc()) {
     addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
     if (DwarfExpr.TagOffset)
       addUInt(*VariableDie, dwarf::DW_AT_LLVM_tag_offset, dwarf::DW_FORM_data1,
               *DwarfExpr.TagOffset);
 
     return VariableDie;
   }
 
-  if (const std::optional<DbgVariableEntryValue> &EntryValueVar =
-          DV.getEntryValue()) {
+  if (const std::set<EntryValueInfo> *EntryValues = DV.getEntryValues()) {
     DIELoc *Loc = new (DIEValueAllocator) DIELoc;
     DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
     // Emit each expression as: EntryValue(Register) <other ops> <Fragment>.
-    for (auto [Register, Expr] : EntryValueVar->getEntryValuesInfo()) {
+    for (auto [Register, Expr] : *EntryValues) {
       DwarfExpr.addFragmentOffset(&Expr);
       DIExpressionCursor Cursor(Expr.getElements());
       DwarfExpr.beginEntryValueExpression(Cursor);
       DwarfExpr.addMachineRegExpression(
           *Asm->MF->getSubtarget().getRegisterInfo(), Cursor, Register);
       DwarfExpr.addExpression(std::move(Cursor));
     }
     addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

 #include "llvm/MC/MCDwarf.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Target/TargetOptions.h"
 #include <cassert>
 #include <cstdint>
 #include <limits>
 #include <memory>
 #include <utility>
+#include <variant>
 #include <vector>
 
 namespace llvm {
 
 class AsmPrinter;
 class ByteStreamer;
 class DIE;
 class DwarfCompileUnit;
static bool classof(const DbgEntity *N) {
     case DbgVariableKind:
     case DbgLabelKind:
       return true;
     }
     llvm_unreachable("Invalid DbgEntityKind");
   }
 };
 
-/// Helper class to model a DbgVariable whose location is derived from an
-/// EntryValue.
-/// TODO: split the current implementation of `DbgVariable` into a class per
-/// variant of location that it can represent, and make `DbgVariableEntryValue`
-/// a subclass.
-class DbgVariableEntryValue {
-  struct EntryValueInfo {
-    MCRegister Reg;
-    const DIExpression &Expr;
+/// Proxy for one MMI entry.
+struct FrameIndexExpr {
+  int FI;
+  const DIExpression *Expr;
+};
+
+/// Represents an entry-value location, or a fragment of one.
 
-    /// Operator enabling sorting based on fragment offset.
-    bool operator<(const EntryValueInfo &Other) const {
-      return getFragmentOffsetInBits() < Other.getFragmentOffsetInBits();
-    }
 
-  private:
-    uint64_t getFragmentOffsetInBits() const {
-      std::optional<DIExpression::FragmentInfo> Fragment =
-          Expr.getFragmentInfo();
-      return Fragment ? Fragment->OffsetInBits : 0;
-    }
-  };
+    std::optional<DIExpression::FragmentInfo> Fragment = Expr.getFragmentInfo();
 
+// Namespace for private alternatives of a DbgVariable.
+namespace {
+/// Single value location description.
+struct SingleLoc {
+  std::unique_ptr<DbgValueLoc> ValueLoc;
+  const DIExpression *Expr;
+  SingleLoc(DbgValueLoc ValueLoc)
+      : ValueLoc(std::make_unique<DbgValueLoc>(ValueLoc)),
+        Expr(ValueLoc.getExpression()) {
+    if (!Expr->getNumElements())
+      Expr = nullptr;
+  }
+};
+/// Multi-value location description.
+struct MultiLoc {
+  /// Index of the entry list in DebugLocs.
+  unsigned DebugLocListIndex = ~0u;
+  /// DW_OP_LLVM_tag_offset value from DebugLocs.
+  std::optional<uint8_t> DebugLocListTagOffset;
+};
+/// Single location defined by (potentially multiple) MMI entries.
+struct MMILoc {
+  mutable SmallVector<FrameIndexExpr, 1> FrameIndexExprs;
+};
+/// Single location defined by (potentially multiple) EntryValueInfo.
+struct EntryValueLoc {
   std::set<EntryValueInfo> EntryValues;
-
-public:
-  DbgVariableEntryValue(MCRegister Reg, const DIExpression &Expr) {
+  EntryValueLoc(MCRegister Reg, const DIExpression &Expr) {
     addExpr(Reg, Expr);
   };
-
   // Add the pair Reg, Expr to the list of entry values describing the variable.
   // If multiple expressions are added, it is the callers responsibility to
   // ensure they are all non-overlapping fragments.
   void addExpr(MCRegister Reg, const DIExpression &Expr) {
     std::optional<const DIExpression *> NonVariadicExpr =
         DIExpression::convertToNonVariadicExpression(&Expr);
     assert(NonVariadicExpr && *NonVariadicExpr);
 
     EntryValues.insert({Reg, **NonVariadicExpr});
   }
-
-  /// Returns the set of EntryValueInfo.
-  const std::set<EntryValueInfo> &getEntryValuesInfo() const {
-    return EntryValues;
-  }
 };
+} // namespace
 
 //===----------------------------------------------------------------------===//
 /// This class is used to track local variable information.
 ///
+/// Variables that have been optimized out hold the \c monostate alternative.
+/// This is not distinguished from the case of a constructed \c DbgVariable
+/// which has not be initialized yet.
+///
 /// Variables can be created from allocas, in which case they're generated from
-/// the MMI table.  Such variables can have multiple expressions and frame
-/// indices.
+/// the MMI table. Such variables hold the \c MMILoc alternative which can have
+/// multiple expressions and frame indices.
 ///
 /// Variables can be created from the entry value of registers, in which case
-/// they're generated from the MMI table. Such variables can have either a
-/// single expression or multiple *fragment* expressions.
+/// they're generated from the MMI table. Such variables hold the \c
+/// EntryValueLoc alternative which can either have a single expression or
+/// multiple *fragment* expressions.
 ///
-/// Variables can be created from \c DBG_VALUE instructions.  Those whose
-/// location changes over time use \a DebugLocListIndex, while those with a
-/// single location use \a ValueLoc and (optionally) a single entry of \a Expr.
-///
-/// Variables that have been optimized out use none of these fields.
-class DbgVariable : public DbgEntity {
-  /// Index of the entry list in DebugLocs.
-  unsigned DebugLocListIndex = ~0u;
-  /// DW_OP_LLVM_tag_offset value from DebugLocs.
-  std::optional<uint8_t> DebugLocListTagOffset;
-
-  /// Single value location description.
-  std::unique_ptr<DbgValueLoc> ValueLoc = nullptr;
-
-  struct FrameIndexExpr {
-    int FI;
-    const DIExpression *Expr;
-  };
-  mutable SmallVector<FrameIndexExpr, 1>
-      FrameIndexExprs; /// Frame index + expression.
-
-  std::optional<DbgVariableEntryValue> EntryValue;
+/// Variables can be created from \c DBG_VALUE instructions. Those whose
+/// location changes over time hold a \c MultiLoc alternative which uses \c
+/// DebugLocListIndex and (optionally) \c DebugLocListTagOffset, while those
+/// with a single location hold a \c SingleLoc alternative which use \c ValueLoc
+/// and (optionally) a single \c Expr.
+class DbgVariable : public DbgEntity,
+                    private std::variant<std::monostate, SingleLoc, MultiLoc,
+                                         MMILoc, EntryValueLoc> {
+
+  /// Member shorthand for std::holds_alternative
+  template <typename T> bool holds() const {
+    return std::holds_alternative<T>(*this);
+  }
+  /// Asserting, noexcept member alternative to std::get
+  template <typename T> auto &get() noexcept {
+    assert(holds<T>());
+    return *std::get_if<T>(this);
+  }
+  /// Asserting, noexcept member alternative to std::get
+  template <typename T> const auto &get() const noexcept {
+    assert(holds<T>());
+    return *std::get_if<T>(this);
+  }
 
 public:
   /// Construct a DbgVariable.
   ///
   /// Creates a variable without any DW_AT_location.  Call \a initializeMMI()
   /// for MMI entries, or \a initializeDbgValue() for DBG_VALUE instructions.
   DbgVariable(const DILocalVariable *V, const DILocation *IA)
       : DbgEntity(V, IA, DbgVariableKind) {}
 
-  bool isInitialized() const {
-    return !FrameIndexExprs.empty() || ValueLoc || EntryValue;
-  }
-
   /// Initialize from the MMI table.
-  void initializeMMI(const DIExpression *E, int FI) {
-    assert(!isInitialized() && "Already initialized?");
-
-    assert((!E || E->isValid()) && "Expected valid expression");
-    assert(FI != std::numeric_limits<int>::max() && "Expected valid index");
-
-    FrameIndexExprs.push_back({FI, E});
-  }
+  void initializeMMI(const DIExpression *E, int FI);
 
   // Initialize variable's location.
-  void initializeDbgValue(DbgValueLoc Value) {
-    assert(!isInitialized() && "Already initialized?");
-    assert(!Value.getExpression()->isFragment() && "Fragments not supported.");
+  void initializeDbgValue(DbgValueLoc Value);
 
-    ValueLoc = std::make_unique<DbgValueLoc>(Value);
-    if (auto *E = ValueLoc->getExpression())
-      if (E->getNumElements())
-        FrameIndexExprs.push_back({0, E});
-  }
+  void initializeEntryValue(MCRegister Reg, const DIExpression &Expr);
 
-  void initializeEntryValue(MCRegister Reg, const DIExpression &Expr) {
-    assert(!isInitialized() && "Already initialized?");
-    EntryValue = DbgVariableEntryValue(Reg, Expr);
+  const std::set<EntryValueInfo> *getEntryValues() const {
+    if (const auto *EV = std::get_if<EntryValueLoc>(this))
+      return &EV->EntryValues;
+    return nullptr;
+  }
+  std::set<EntryValueInfo> *getEntryValues() {
+    if (auto *EV = std::get_if<EntryValueLoc>(this))
+      return &EV->EntryValues;
+    return nullptr;
   }
-
-  const std::optional<DbgVariableEntryValue> &getEntryValue() const {
-    return EntryValue;
+  void addEntryValueExpr(MCRegister Reg, const DIExpression &Expr) {
+    get<EntryValueLoc>().addExpr(Reg, Expr);
   }
 
-  std::optional<DbgVariableEntryValue> &getEntryValue() { return EntryValue; }
-
   /// Initialize from a DBG_VALUE instruction.
   void initializeDbgValue(const MachineInstr *DbgValue);
 
   // Accessors.
   const DILocalVariable *getVariable() const {
     return cast<DILocalVariable>(getEntity());
   }
 
   const DIExpression *getSingleExpression() const {
-    assert(ValueLoc.get() && FrameIndexExprs.size() <= 1);
-    return FrameIndexExprs.size() ? FrameIndexExprs[0].Expr : nullptr;
+    return get<SingleLoc>().Expr;
   }
 
-  void setDebugLocListIndex(unsigned O) { DebugLocListIndex = O; }
-  unsigned getDebugLocListIndex() const { return DebugLocListIndex; }
-  void setDebugLocListTagOffset(uint8_t O) { DebugLocListTagOffset = O; }
+  void setDebugLocListIndex(uint8_t O) {
+    if (auto *M = std::get_if<MultiLoc>(this))
+      M->DebugLocListIndex = O;
+    else
+      emplace<MultiLoc>().DebugLocListIndex = O;
+  }
+  unsigned getDebugLocListIndex() const {
+    if (auto *M = std::get_if<MultiLoc>(this))
+      return M->DebugLocListIndex;
+    return ~0U;
+  }
+  void setDebugLocListTagOffset(uint8_t O) {
+    if (auto *M = std::get_if<MultiLoc>(this))
+      M->DebugLocListTagOffset = O;
+    else
+      emplace<MultiLoc>().DebugLocListTagOffset = O;
+  }
   std::optional<uint8_t> getDebugLocListTagOffset() const {
-    return DebugLocListTagOffset;
+    return get<MultiLoc>().DebugLocListTagOffset;
   }
   StringRef getName() const { return getVariable()->getName(); }
-  const DbgValueLoc *getValueLoc() const { return ValueLoc.get(); }
+  const DbgValueLoc *getValueLoc() const {
+    if (auto *M = std::get_if<SingleLoc>(this))
+      return M->ValueLoc.get();
+    return nullptr;
+  }
   /// Get the FI entries, sorted by fragment offset.
   ArrayRef<FrameIndexExpr> getFrameIndexExprs() const;
-  bool hasFrameIndexExprs() const { return !FrameIndexExprs.empty(); }
+  bool hasFrameIndexExprs() const { return holds<MMILoc>(); }
   void addMMIEntry(const DbgVariable &V);
 
   // Translate tag to proper Dwarf tag.
   dwarf::Tag getTag() const {
     // FIXME: Why don't we just infer this tag and store it all along?
     if (getVariable()->isParameter())
       return dwarf::DW_TAG_formal_parameter;
 
public:
   bool isObjectPointer() const {
     if (getVariable()->isObjectPointer())
       return true;
     if (getType()->isObjectPointer())
       return true;
     return false;
   }
 
-  bool hasComplexAddress() const {
-    assert(ValueLoc.get() && "Expected DBG_VALUE, not MMI variable");
-    assert((FrameIndexExprs.empty() ||
-            (FrameIndexExprs.size() == 1 &&
-             FrameIndexExprs[0].Expr->getNumElements())) &&
-           "Invalid Expr for DBG_VALUE");
-    return !FrameIndexExprs.empty();
-  }
+  bool hasComplexAddress() const { return get<SingleLoc>().Expr; }
 
   const DIType *getType() const;
 
   static bool classof(const DbgEntity *N) {
     return N->getDbgEntityID() == DbgVariableKind;
   }
 };
 

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

for (const MachineOperand &Op : MI->debug_operands()) {
     else if (Op.isCImm())
       DbgValueLocEntries.push_back(DbgValueLocEntry(Op.getCImm()));
     else
       llvm_unreachable("Unexpected debug operand in DBG_VALUE* instruction!");
   }
   return DbgValueLoc(Expr, DbgValueLocEntries, IsVariadic);
 }
 
-void DbgVariable::initializeDbgValue(const MachineInstr *DbgValue) {
-  assert(FrameIndexExprs.empty() && "Already initialized?");
-  assert(!ValueLoc.get() && "Already initialized?");
+/// Initialize from the MMI table.
+void DbgVariable::initializeMMI(const DIExpression *E, int FI) {
+  assert(holds<std::monostate>() && "Already initialized");
+  assert((!E || E->isValid()) && "Expected valid expression");
+  assert(FI != std::numeric_limits<int>::max() && "Expected valid index");
+  emplace<MMILoc>().FrameIndexExprs.push_back({FI, E});
+}
+
+void DbgVariable::initializeDbgValue(DbgValueLoc Value) {
+  assert(holds<std::monostate>() && "Already initialized");
+  assert(!Value.getExpression()->isFragment() && "Fragments not supported");
+  emplace<SingleLoc>(Value);
+}
 
+void DbgVariable::initializeDbgValue(const MachineInstr *DbgValue) {
+  assert(holds<std::monostate>() && "Already initialized");
   assert(getVariable() == DbgValue->getDebugVariable() && "Wrong variable");
   assert(getInlinedAt() == DbgValue->getDebugLoc()->getInlinedAt() &&
          "Wrong inlined-at");
+  emplace<SingleLoc>(getDebugLocValue(DbgValue));
+}
 
-  ValueLoc = std::make_unique<DbgValueLoc>(getDebugLocValue(DbgValue));
-  if (auto *E = DbgValue->getDebugExpression())
-    if (E->getNumElements())
-      FrameIndexExprs.push_back({0, E});
+void DbgVariable::initializeEntryValue(MCRegister Reg,
+                                       const DIExpression &Expr) {
+  assert(holds<std::monostate>() && "Already initialized?");
+  emplace<EntryValueLoc>(Reg, Expr);
 }
 
-ArrayRef<DbgVariable::FrameIndexExpr> DbgVariable::getFrameIndexExprs() const {
+ArrayRef<FrameIndexExpr> DbgVariable::getFrameIndexExprs() const {
+  auto &FrameIndexExprs = get<MMILoc>().FrameIndexExprs;
+
   if (FrameIndexExprs.size() == 1)
     return FrameIndexExprs;
 
   assert(llvm::all_of(FrameIndexExprs,
                       [](const FrameIndexExpr &A) {
                         return A.Expr->isFragment();
                       }) &&
          "multiple FI expressions without DW_OP_LLVM_fragment");
   llvm::sort(FrameIndexExprs,
              [](const FrameIndexExpr &A, const FrameIndexExpr &B) -> bool {
                return A.Expr->getFragmentInfo()->OffsetInBits <
                       B.Expr->getFragmentInfo()->OffsetInBits;
              });
 
   return FrameIndexExprs;
 }
 
 void DbgVariable::addMMIEntry(const DbgVariable &V) {
-  assert(DebugLocListIndex == ~0U && !ValueLoc.get() && "not an MMI entry");
-  assert(V.DebugLocListIndex == ~0U && !V.ValueLoc.get() && "not an MMI entry");
   assert(V.getVariable() == getVariable() && "conflicting variable");
   assert(V.getInlinedAt() == getInlinedAt() && "conflicting inlined-at location");
 
-  assert(!FrameIndexExprs.empty() && "Expected an MMI entry");
-  assert(!V.FrameIndexExprs.empty() && "Expected an MMI entry");
+  auto &FrameIndexExprs = get<MMILoc>().FrameIndexExprs;
+  auto &VFrameIndexExprs = V.get<MMILoc>().FrameIndexExprs;
 
   // FIXME: This logic should not be necessary anymore, as we now have proper
   // deduplication. However, without it, we currently run into the assertion
   // below, which means that we are likely dealing with broken input, i.e. two
   // non-fragment entries for the same variable at different frame indices.
   if (FrameIndexExprs.size()) {
     auto *Expr = FrameIndexExprs.back().Expr;
     if (!Expr || !Expr->isFragment())
       return;
   }
 
-  for (const auto &FIE : V.FrameIndexExprs)
+  for (const auto &FIE : VFrameIndexExprs)
     // Ignore duplicate entries.
     if (llvm::none_of(FrameIndexExprs, [&](const FrameIndexExpr &Other) {
           return FIE.FI == Other.FI && FIE.Expr == Other.Expr;
         }))
       FrameIndexExprs.push_back(FIE);
 
   assert((FrameIndexExprs.size() == 1 ||
           llvm::all_of(FrameIndexExprs,
else
       RegVar->initializeEntryValue(VI.getEntryValueRegister(), *VI.Expr);
     LLVM_DEBUG(dbgs() << "Created DbgVariable for " << VI.Var->getName()
                       << "\n");
 
     if (DbgVariable *DbgVar = MFVars.lookup(Var)) {
       if (DbgVar->hasFrameIndexExprs())
         DbgVar->addMMIEntry(*RegVar);
       else
-        DbgVar->getEntryValue()->addExpr(VI.getEntryValueRegister(), *VI.Expr);
+        DbgVar->addEntryValueExpr(VI.getEntryValueRegister(), *VI.Expr);
     } else if (InfoHolder.addScopeVariable(Scope, RegVar.get())) {
       MFVars.insert({Var, RegVar.get()});
       ConcreteEntities.push_back(std::move(RegVar));
     }
   }
 }
 
 /// Determine whether a *singular* DBG_VALUE is valid for the entirety of its