[NFC][AsmPrinter] Use std::visit in constructVariableDIEImpl

[scott.linder]

Summary

This potentially has a slightly positive performance impact, as
std::visit can be implemented as a switch-like jump rather than
a series of ifs.

More importantly, the reader can be confident is no overlap between the
cases.

[fdeazeve]

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

776

What do you think of implementing each of the visitors as a private member of DwarfCompileUnit?
I personally think this function could benefit from being split into different functions for readability, but this is a larger refactor.

[scott.linder]

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

776

I definitely considered it, and agree it is worthwhile!

The whole series was an attempt to keep the changes gradual and understandable, and to leave open the option to drop things piecemeal. I try to assume the original intent has some rationale I am not considering, as that is often the case anyway (a lot of smart, dedicated people have contributed here) and I always worry that I'm changing things just to change them.

Hearing you come to the same conclusion makes me confident it is an improvement, though, so I will post a follow up!

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

DIE *DwarfCompileUnit::constructVariableDIEImpl(const DbgVariable &DV,
   insertDIE(DV.getVariable(), VariableDie);
 
   if (Abstract) {
     applyVariableAttributes(DV, *VariableDie);
     return VariableDie;
   }
 
   // Add variable address.
-
-  if (auto *Multi = std::get_if<Loc::Multi>(&DV)) {
-    addLocationList(*VariableDie, dwarf::DW_AT_location,
-                    Multi->getDebugLocListIndex());
-    auto TagOffset = Multi->getDebugLocListTagOffset();
-    if (TagOffset)
-      addUInt(*VariableDie, dwarf::DW_AT_LLVM_tag_offset, dwarf::DW_FORM_data1,
-              *TagOffset);
-    return VariableDie;
-  }
-
-  // Check if variable has a single location description.
-  if (const auto *Single = std::get_if<Loc::Single>(&DV)) {
-    const DbgValueLoc *DVal = &Single->getValueLoc();
+  std::visit(
+      makeVisitor(
+          [=, &DV](const Loc::Single &Single) {
+            const DbgValueLoc *DVal = &Single.getValueLoc();
             if (!DVal->isVariadic()) {
               const DbgValueLocEntry *Entry = DVal->getLocEntries().begin();
               if (Entry->isLocation()) {
                 addVariableAddress(DV, *VariableDie, Entry->getLoc());
               } else if (Entry->isInt()) {
-        auto *Expr = Single->getExpr();
+                auto *Expr = Single.getExpr();
                 if (Expr && Expr->getNumElements()) {
                   DIELoc *Loc = new (DIEValueAllocator) DIELoc;
                   DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
                   // If there is an expression, emit raw unsigned bytes.
                   DwarfExpr.addFragmentOffset(Expr);
                   DwarfExpr.addUnsignedConstant(Entry->getInt());
                   DwarfExpr.addExpression(Expr);
-          addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
+                  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);
                 } else
                   addConstantValue(*VariableDie, Entry->getInt(), DV.getType());
               } else if (Entry->isConstantFP()) {
                 addConstantFPValue(*VariableDie, Entry->getConstantFP());
               } else if (Entry->isConstantInt()) {
-        addConstantValue(*VariableDie, Entry->getConstantInt(), DV.getType());
+                addConstantValue(*VariableDie, Entry->getConstantInt(),
+                                 DV.getType());
               } else if (Entry->isTargetIndexLocation()) {
                 DIELoc *Loc = new (DIEValueAllocator) DIELoc;
                 DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
                 const DIBasicType *BT = dyn_cast<DIBasicType>(
                     static_cast<const Metadata *>(DV.getVariable()->getType()));
                 DwarfDebug::emitDebugLocValue(*Asm, BT, *DVal, DwarfExpr);
-        addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
+                addBlock(*VariableDie, dwarf::DW_AT_location,
+                         DwarfExpr.finalize());
               }
-      return VariableDie;
+              return;
             }
-    // If any of the location entries are registers with the value 0, then the
-    // location is undefined.
-    if (any_of(DVal->getLocEntries(), [](const DbgValueLocEntry &Entry) {
+            // If any of the location entries are registers with the value 0,
+            // then the location is undefined.
+            if (any_of(DVal->getLocEntries(),
+                       [](const DbgValueLocEntry &Entry) {
                          return Entry.isLocation() && !Entry.getLoc().getReg();
                        }))
-      return VariableDie;
-    const DIExpression *Expr = Single->getExpr();
+              return;
+            const DIExpression *Expr = Single.getExpr();
             assert(Expr && "Variadic Debug Value must have an Expression.");
             DIELoc *Loc = new (DIEValueAllocator) DIELoc;
             DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
             DwarfExpr.addFragmentOffset(Expr);
             DIExpressionCursor Cursor(Expr);
-    const TargetRegisterInfo &TRI = *Asm->MF->getSubtarget().getRegisterInfo();
+            const TargetRegisterInfo &TRI =
+                *Asm->MF->getSubtarget().getRegisterInfo();
 
             auto AddEntry = [&](const DbgValueLocEntry &Entry,
                                 DIExpressionCursor &Cursor) {
               if (Entry.isLocation()) {
                 if (!DwarfExpr.addMachineRegExpression(TRI, Cursor,
                                                        Entry.getLoc().getReg()))
                   return false;
               } else if (Entry.isInt()) {
                 // If there is an expression, emit raw unsigned bytes.
                 DwarfExpr.addUnsignedConstant(Entry.getInt());
               } else if (Entry.isConstantFP()) {
                 // DwarfExpression does not support arguments wider than 64 bits
                 // (see PR52584).
                 // TODO: Consider chunking expressions containing overly wide
                 // arguments into separate pointer-sized fragment expressions.
-        APInt RawBytes = Entry.getConstantFP()->getValueAPF().bitcastToAPInt();
+                APInt RawBytes =
+                    Entry.getConstantFP()->getValueAPF().bitcastToAPInt();
                 if (RawBytes.getBitWidth() > 64)
                   return false;
                 DwarfExpr.addUnsignedConstant(RawBytes.getZExtValue());
               } else if (Entry.isConstantInt()) {
                 APInt RawBytes = Entry.getConstantInt()->getValue();
                 if (RawBytes.getBitWidth() > 64)
                   return false;
                 DwarfExpr.addUnsignedConstant(RawBytes.getZExtValue());
               } else if (Entry.isTargetIndexLocation()) {
                 TargetIndexLocation Loc = Entry.getTargetIndexLocation();
-        // TODO TargetIndexLocation is a target-independent. Currently only the
-        // WebAssembly-specific encoding is supported.
+                // TODO TargetIndexLocation is a target-independent. Currently
+                // only the WebAssembly-specific encoding is supported.
                 assert(Asm->TM.getTargetTriple().isWasm());
-        DwarfExpr.addWasmLocation(Loc.Index, static_cast<uint64_t>(Loc.Offset));
+                DwarfExpr.addWasmLocation(Loc.Index,
+                                          static_cast<uint64_t>(Loc.Offset));
               } else {
                 llvm_unreachable("Unsupported Entry type.");
               }
               return true;
             };
 
             if (!DwarfExpr.addExpression(
                     std::move(Cursor),
                     [&](unsigned Idx, DIExpressionCursor &Cursor) -> bool {
                       return AddEntry(DVal->getLocEntries()[Idx], Cursor);
                     }))
-      return VariableDie;
+              return;
 
             // Now attach the location information to the DIE.
             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 (auto *EntryValue = std::get_if<Loc::EntryValue>(&DV)) {
-    DIELoc *Loc = new (DIEValueAllocator) DIELoc;
-    DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
-    // Emit each expression as: EntryValue(Register) <other ops> <Fragment>.
-    for (auto [Register, Expr] : EntryValue->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());
-    return VariableDie;
-  }
-
-  // .. else use frame index.
-  if (!DV.holds<Loc::MMI>())
-    return VariableDie;
-
+              addUInt(*VariableDie, dwarf::DW_AT_LLVM_tag_offset,
+                      dwarf::DW_FORM_data1, *DwarfExpr.TagOffset);
+          },
+          [=](const Loc::Multi &Multi) {
+            addLocationList(*VariableDie, dwarf::DW_AT_location,
+                            Multi.getDebugLocListIndex());
+            auto TagOffset = Multi.getDebugLocListTagOffset();
+            if (TagOffset)
+              addUInt(*VariableDie, dwarf::DW_AT_LLVM_tag_offset,
+                      dwarf::DW_FORM_data1, *TagOffset);
+          },
+          [=](const Loc::MMI &MMI) {
             std::optional<unsigned> NVPTXAddressSpace;
             DIELoc *Loc = new (DIEValueAllocator) DIELoc;
             DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
-  for (const auto &Fragment : DV.get<Loc::MMI>().getFrameIndexExprs()) {
+            for (const auto &Fragment : MMI.getFrameIndexExprs()) {
               Register FrameReg;
               const DIExpression *Expr = Fragment.Expr;
-    const TargetFrameLowering *TFI = Asm->MF->getSubtarget().getFrameLowering();
+              const TargetFrameLowering *TFI =
+                  Asm->MF->getSubtarget().getFrameLowering();
               StackOffset Offset =
                   TFI->getFrameIndexReference(*Asm->MF, Fragment.FI, FrameReg);
               DwarfExpr.addFragmentOffset(Expr);
 
               auto *TRI = Asm->MF->getSubtarget().getRegisterInfo();
               SmallVector<uint64_t, 8> Ops;
               TRI->getOffsetOpcodes(Offset, Ops);
 
               // According to
               // https://docs.nvidia.com/cuda/archive/10.0/ptx-writers-guide-to-interoperability/index.html#cuda-specific-dwarf
-    // cuda-gdb requires DW_AT_address_class for all variables to be able to
-    // correctly interpret address space of the variable address.
-    // Decode DW_OP_constu <DWARF Address Space> DW_OP_swap DW_OP_xderef
-    // sequence for the NVPTX + gdb target.
+              // cuda-gdb requires DW_AT_address_class for all variables to be
+              // able to correctly interpret address space of the variable
+              // address. Decode DW_OP_constu <DWARF Address Space> DW_OP_swap
+              // DW_OP_xderef sequence for the NVPTX + gdb target.
               unsigned LocalNVPTXAddressSpace;
               if (Asm->TM.getTargetTriple().isNVPTX() && DD->tuneForGDB()) {
-      const DIExpression *NewExpr =
-          DIExpression::extractAddressClass(Expr, LocalNVPTXAddressSpace);
+                const DIExpression *NewExpr = DIExpression::extractAddressClass(
+                    Expr, LocalNVPTXAddressSpace);
                 if (NewExpr != Expr) {
                   Expr = NewExpr;
                   NVPTXAddressSpace = LocalNVPTXAddressSpace;
                 }
               }
               if (Expr)
                 Ops.append(Expr->elements_begin(), Expr->elements_end());
               DIExpressionCursor Cursor(Ops);
               DwarfExpr.setMemoryLocationKind();
               if (const MCSymbol *FrameSymbol = Asm->getFunctionFrameSymbol())
                 addOpAddress(*Loc, FrameSymbol);
               else
                 DwarfExpr.addMachineRegExpression(
-          *Asm->MF->getSubtarget().getRegisterInfo(), Cursor, FrameReg);
+                    *Asm->MF->getSubtarget().getRegisterInfo(), Cursor,
+                    FrameReg);
               DwarfExpr.addExpression(std::move(Cursor));
             }
             if (Asm->TM.getTargetTriple().isNVPTX() && DD->tuneForGDB()) {
               // According to
               // https://docs.nvidia.com/cuda/archive/10.0/ptx-writers-guide-to-interoperability/index.html#cuda-specific-dwarf
-    // cuda-gdb requires DW_AT_address_class for all variables to be able to
-    // correctly interpret address space of the variable address.
+              // cuda-gdb requires DW_AT_address_class for all variables to be
+              // able to correctly interpret address space of the variable
+              // address.
               const unsigned NVPTX_ADDR_local_space = 6;
-    addUInt(*VariableDie, dwarf::DW_AT_address_class, dwarf::DW_FORM_data1,
+              addUInt(*VariableDie, dwarf::DW_AT_address_class,
+                      dwarf::DW_FORM_data1,
                       NVPTXAddressSpace.value_or(NVPTX_ADDR_local_space));
             }
             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);
+              addUInt(*VariableDie, dwarf::DW_AT_LLVM_tag_offset,
+                      dwarf::DW_FORM_data1, *DwarfExpr.TagOffset);
+          },
+          [=](const Loc::EntryValue &EntryValue) {
+            DIELoc *Loc = new (DIEValueAllocator) DIELoc;
+            DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
+            // Emit each expression as: EntryValue(Register) <other ops>
+            // <Fragment>.
+            for (auto [Register, Expr] : EntryValue.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());
+          },
+          [](const std::monostate &) {}),
+      DV.asVariant());
 
   return VariableDie;
 }
 
 DIE *DwarfCompileUnit::constructVariableDIE(DbgVariable &DV,
                                             const LexicalScope &Scope,
                                             DIE *&ObjectPointer) {
   auto Var = constructVariableDIE(DV, Scope.isAbstractScope());

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h

struct EntryValue {
   void addExpr(MCRegister Reg, const DIExpression &Expr) {
     std::optional<const DIExpression *> NonVariadicExpr =
         DIExpression::convertToNonVariadicExpression(&Expr);
     assert(NonVariadicExpr && *NonVariadicExpr);
 
     EntryValues.insert({Reg, **NonVariadicExpr});
   }
 };
+/// Alias for the std::variant specialization base class of DbgVariable.
+using Variant = std::variant<std::monostate, Loc::Single, Loc::Multi, Loc::MMI,
+                             Loc::EntryValue>;
 } // namespace Loc
 
 //===----------------------------------------------------------------------===//
 /// 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 hold the \c Loc::MMI 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 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 hold a \c Loc::Multi alternative which uses \c
 /// DebugLocListIndex and (optionally) \c DebugLocListTagOffset, while those
 /// with a single location hold a \c Loc::Single alternative which use \c
 /// ValueLoc and (optionally) a single \c Expr.
-class DbgVariable : public DbgEntity,
-                    public std::variant<std::monostate, Loc::Single, Loc::Multi,
-                                        Loc::MMI, Loc::EntryValue> {
+class DbgVariable : public DbgEntity, public Loc::Variant {
 
 public:
+  /// To workaround P2162R0 https://github.com/cplusplus/papers/issues/873 the
+  /// base class subobject needs to be passed directly to std::visit, so expose
+  /// it directly here.
+  Loc::Variant &asVariant() { return *static_cast<Loc::Variant *>(this); }
+  const Loc::Variant &asVariant() const {
+    return *static_cast<const Loc::Variant *>(this);
+  }
   /// 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);