Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DebugInfo] Make DIExpression inherit from Metadata and it always should be unique #79335

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

phyBrackets
Copy link
Member

DIExpression no longer inherits from MDNode. This change was motivated by the need to ensure that DIExpression instances have a unique storage type and there was a bit discussion over on discourse .

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 24, 2024

@llvm/pr-subscribers-llvm-selectiondag
@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-llvm-globalisel
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-debuginfo

Author: Shivam (phyBrackets)

Changes

DIExpression no longer inherits from MDNode. This change was motivated by the need to ensure that DIExpression instances have a unique storage type and there was a bit discussion over on discourse .


Patch is 30.18 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/79335.diff

20 Files Affected:

  • (modified) llvm/include/llvm/AsmParser/LLParser.h (+1)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h (+4-4)
  • (modified) llvm/include/llvm/CodeGen/MachineInstrBuilder.h (+15-4)
  • (modified) llvm/include/llvm/CodeGen/MachineOperand.h (+14-2)
  • (modified) llvm/include/llvm/IR/DebugInfoMetadata.h (+11-15)
  • (modified) llvm/include/llvm/IR/Metadata.def (+1-1)
  • (modified) llvm/lib/AsmParser/LLParser.cpp (+10-12)
  • (modified) llvm/lib/Bitcode/Reader/MetadataLoader.cpp (+1-2)
  • (modified) llvm/lib/Bitcode/Writer/BitcodeWriter.cpp (+8-5)
  • (modified) llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp (+4-4)
  • (modified) llvm/lib/CodeGen/MIRParser/MIParser.cpp (+6-9)
  • (modified) llvm/lib/CodeGen/MachineInstr.cpp (+4-4)
  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp (+1-1)
  • (modified) llvm/lib/IR/DIBuilder.cpp (-2)
  • (modified) llvm/lib/IR/DebugInfoMetadata.cpp (+14-5)
  • (modified) llvm/lib/IR/LLVMContextImpl.h (+46)
  • (modified) llvm/lib/IR/Verifier.cpp (+1)
  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp (+1-1)
  • (modified) llvm/lib/Transforms/Utils/ValueMapper.cpp (+10-2)
  • (modified) llvm/unittests/IR/MetadataTest.cpp (-3)
diff --git a/llvm/include/llvm/AsmParser/LLParser.h b/llvm/include/llvm/AsmParser/LLParser.h
index cf358c384f52033..af4423cb4a7ca15 100644
--- a/llvm/include/llvm/AsmParser/LLParser.h
+++ b/llvm/include/llvm/AsmParser/LLParser.h
@@ -579,6 +579,7 @@ namespace llvm {
     bool parseValueAsMetadata(Metadata *&MD, const Twine &TypeMsg,
                               PerFunctionState *PFS);
     bool parseDIArgList(Metadata *&MD, PerFunctionState *PFS);
+    bool parseDIExpression(Metadata *&MD);
     bool parseMetadata(Metadata *&MD, PerFunctionState *PFS);
     bool parseMDTuple(MDNode *&MD, bool IsDistinct = false);
     bool parseMDNode(MDNode *&N);
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
index 1387a0a37561c4b..2691188d9cffc17 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
@@ -410,26 +410,26 @@ class MachineIRBuilder {
   /// Build and insert a DBG_VALUE instruction expressing the fact that the
   /// associated \p Variable lives in \p Reg (suitably modified by \p Expr).
   MachineInstrBuilder buildDirectDbgValue(Register Reg, const MDNode *Variable,
-                                          const MDNode *Expr);
+                                          const Metadata *Expr);
 
   /// Build and insert a DBG_VALUE instruction expressing the fact that the
   /// associated \p Variable lives in memory at \p Reg (suitably modified by \p
   /// Expr).
   MachineInstrBuilder buildIndirectDbgValue(Register Reg,
                                             const MDNode *Variable,
-                                            const MDNode *Expr);
+                                            const Metadata *Expr);
 
   /// Build and insert a DBG_VALUE instruction expressing the fact that the
   /// associated \p Variable lives in the stack slot specified by \p FI
   /// (suitably modified by \p Expr).
   MachineInstrBuilder buildFIDbgValue(int FI, const MDNode *Variable,
-                                      const MDNode *Expr);
+                                      const Metadata *Expr);
 
   /// Build and insert a DBG_VALUE instructions specifying that \p Variable is
   /// given by \p C (suitably modified by \p Expr).
   MachineInstrBuilder buildConstDbgValue(const Constant &C,
                                          const MDNode *Variable,
-                                         const MDNode *Expr);
+                                         const Metadata *Expr);
 
   /// Build and insert a DBG_LABEL instructions specifying that \p Label is
   /// given. Convert "llvm.dbg.label Label" to "DBG_LABEL Label".
diff --git a/llvm/include/llvm/CodeGen/MachineInstrBuilder.h b/llvm/include/llvm/CodeGen/MachineInstrBuilder.h
index 954d8e6770a294f..c0c2a4ea4c0c23a 100644
--- a/llvm/include/llvm/CodeGen/MachineInstrBuilder.h
+++ b/llvm/include/llvm/CodeGen/MachineInstrBuilder.h
@@ -245,6 +245,17 @@ class MachineInstrBuilder {
     return *this;
   }
 
+  const MachineInstrBuilder &addMetadata(const Metadata *MD) const {
+    MI->addOperand(*MF, MachineOperand::CreateMetadata(MD));
+    assert((MI->isDebugValueLike() ? static_cast<bool>(MI->getDebugVariable())
+                                   : true) &&
+           "first MDNode argument of a DBG_VALUE not a variable");
+    assert((MI->isDebugLabel() ? static_cast<bool>(MI->getDebugLabel())
+                               : true) &&
+           "first MDNode argument of a DBG_LABEL not a label");
+    return *this;
+  }
+
   const MachineInstrBuilder &addCFIIndex(unsigned CFIIndex) const {
     MI->addOperand(*MF, MachineOperand::CreateCFIIndex(CFIIndex));
     return *this;
@@ -488,14 +499,14 @@ inline MachineInstrBuilder BuildMI(MachineBasicBlock *BB,
 MachineInstrBuilder BuildMI(MachineFunction &MF, const DebugLoc &DL,
                             const MCInstrDesc &MCID, bool IsIndirect,
                             Register Reg, const MDNode *Variable,
-                            const MDNode *Expr);
+                            const Metadata *Expr);
 
 /// This version of the builder builds a DBG_VALUE or DBG_VALUE_LIST intrinsic
 /// for a MachineOperand.
 MachineInstrBuilder BuildMI(MachineFunction &MF, const DebugLoc &DL,
                             const MCInstrDesc &MCID, bool IsIndirect,
                             ArrayRef<MachineOperand> MOs,
-                            const MDNode *Variable, const MDNode *Expr);
+                            const MDNode *Variable, const Metadata *Expr);
 
 /// This version of the builder builds a DBG_VALUE intrinsic
 /// for either a value in a register or a register-indirect
@@ -504,7 +515,7 @@ MachineInstrBuilder BuildMI(MachineBasicBlock &BB,
                             MachineBasicBlock::iterator I, const DebugLoc &DL,
                             const MCInstrDesc &MCID, bool IsIndirect,
                             Register Reg, const MDNode *Variable,
-                            const MDNode *Expr);
+                            const Metadata *Expr);
 
 /// This version of the builder builds a DBG_VALUE, DBG_INSTR_REF, or
 /// DBG_VALUE_LIST intrinsic for a machine operand and inserts it at position I.
@@ -512,7 +523,7 @@ MachineInstrBuilder BuildMI(MachineBasicBlock &BB,
                             MachineBasicBlock::iterator I, const DebugLoc &DL,
                             const MCInstrDesc &MCID, bool IsIndirect,
                             ArrayRef<MachineOperand> MOs,
-                            const MDNode *Variable, const MDNode *Expr);
+                            const MDNode *Variable, const Metadata *Expr);
 
 /// Clone a DBG_VALUE whose value has been spilled to FrameIndex.
 MachineInstr *buildDbgValueForSpill(MachineBasicBlock &BB,
diff --git a/llvm/include/llvm/CodeGen/MachineOperand.h b/llvm/include/llvm/CodeGen/MachineOperand.h
index 63a172134538c42..9d5532c7d812ec1 100644
--- a/llvm/include/llvm/CodeGen/MachineOperand.h
+++ b/llvm/include/llvm/CodeGen/MachineOperand.h
@@ -31,6 +31,7 @@ class MachineInstr;
 class MachineRegisterInfo;
 class MCCFIInstruction;
 class MDNode;
+class Metadata;
 class ModuleSlotTracker;
 class TargetIntrinsicInfo;
 class TargetRegisterInfo;
@@ -173,6 +174,7 @@ class MachineOperand {
     int64_t ImmVal;          // For MO_Immediate.
     const uint32_t *RegMask; // For MO_RegisterMask and MO_RegisterLiveOut.
     const MDNode *MD;        // For MO_Metadata.
+    const Metadata* Expr;
     MCSymbol *Sym;           // For MO_MCSymbol.
     unsigned CFIIndex;       // For MO_CFI.
     Intrinsic::ID IntrinsicID; // For MO_IntrinsicID.
@@ -677,6 +679,11 @@ class MachineOperand {
     return Contents.MD;
   }
 
+  const Metadata *getMetadataDI() const {
+    assert(isMetadata() && "Wrong MachineOperand accessor");
+    return Contents.Expr;
+  }
+
   //===--------------------------------------------------------------------===//
   // Mutators for various operand types.
   //===--------------------------------------------------------------------===//
@@ -710,9 +717,9 @@ class MachineOperand {
     Contents.OffsetedInfo.Val.Index = Idx;
   }
 
-  void setMetadata(const MDNode *MD) {
+  void setMetadata(const Metadata *MD) {
     assert(isMetadata() && "Wrong MachineOperand mutator");
-    Contents.MD = MD;
+    Contents.Expr = MD;
   }
 
   void setInstrRefInstrIndex(unsigned InstrIdx) {
@@ -946,6 +953,11 @@ class MachineOperand {
     Op.Contents.MD = Meta;
     return Op;
   }
+  static MachineOperand CreateMetadata(const Metadata *Meta) {
+    MachineOperand Op(MachineOperand::MO_Metadata);
+    Op.Contents.Expr = Meta;
+    return Op;
+  }
 
   static MachineOperand CreateMCSymbol(MCSymbol *Sym,
                                        unsigned TargetFlags = 0) {
diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h
index 156f6eb49253de7..207deed8a78dd97 100644
--- a/llvm/include/llvm/IR/DebugInfoMetadata.h
+++ b/llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -2656,31 +2656,27 @@ class DIVariable : public DINode {
 /// DW_OP_stack_value) is the constant variable value.
 ///
 /// TODO: Co-allocate the expression elements.
-/// TODO: Separate from MDNode, or otherwise drop Distinct and Temporary
-/// storage types.
-class DIExpression : public MDNode {
+class DIExpression : public Metadata, ReplaceableMetadataImpl {
+  friend class ReplaceableMetadataImpl;
   friend class LLVMContextImpl;
-  friend class MDNode;
 
   std::vector<uint64_t> Elements;
 
-  DIExpression(LLVMContext &C, StorageType Storage, ArrayRef<uint64_t> Elements)
-      : MDNode(C, DIExpressionKind, Storage, std::nullopt),
+  DIExpression(LLVMContext &C, ArrayRef<uint64_t> Elements)
+      : Metadata(DIExpressionKind, Uniqued), ReplaceableMetadataImpl(C),
         Elements(Elements.begin(), Elements.end()) {}
   ~DIExpression() = default;
 
   static DIExpression *getImpl(LLVMContext &Context,
-                               ArrayRef<uint64_t> Elements, StorageType Storage,
-                               bool ShouldCreate = true);
-
-  TempDIExpression cloneImpl() const {
-    return getTemporary(getContext(), getElements());
-  }
+                               ArrayRef<uint64_t> Elements);
 
 public:
-  DEFINE_MDNODE_GET(DIExpression, (ArrayRef<uint64_t> Elements), (Elements))
-
-  TempDIExpression clone() const { return cloneImpl(); }
+    LLVMContext &getContext() const {
+        return ReplaceableMetadataImpl::getContext();
+    }
+    static inline DIExpression *get(LLVMContext &Context, ArrayRef<uint64_t> Elements) {
+        return getImpl(Context, Elements);
+    }
 
   ArrayRef<uint64_t> getElements() const { return Elements; }
 
diff --git a/llvm/include/llvm/IR/Metadata.def b/llvm/include/llvm/IR/Metadata.def
index a3cfb9ad6e3e785..0edbe931c75ed8e 100644
--- a/llvm/include/llvm/IR/Metadata.def
+++ b/llvm/include/llvm/IR/Metadata.def
@@ -81,7 +81,7 @@ HANDLE_METADATA_LEAF(DIArgList)
 HANDLE_MDNODE_BRANCH(MDNode)
 HANDLE_MDNODE_LEAF_UNIQUABLE(MDTuple)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DILocation)
-HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIExpression)
+HANDLE_METADATA_LEAF(DIExpression)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIGlobalVariableExpression)
 HANDLE_SPECIALIZED_MDNODE_BRANCH(DINode)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(GenericDINode)
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index d6c5993797de111..e7b06f8c5fef702 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -892,16 +892,7 @@ bool LLParser::parseNamedMetadata() {
   if (Lex.getKind() != lltok::rbrace)
     do {
       MDNode *N = nullptr;
-      // parse DIExpressions inline as a special case. They are still MDNodes,
-      // so they can still appear in named metadata. Remove this logic if they
-      // become plain Metadata.
-      if (Lex.getKind() == lltok::MetadataVar &&
-          Lex.getStrVal() == "DIExpression") {
-        if (parseDIExpression(N, /*IsDistinct=*/false))
-          return true;
-        // DIArgLists should only appear inline in a function, as they may
-        // contain LocalAsMetadata arguments which require a function context.
-      } else if (Lex.getKind() == lltok::MetadataVar &&
+       if (Lex.getKind() == lltok::MetadataVar &&
                  Lex.getStrVal() == "DIArgList") {
         return tokError("found DIArgList outside of function");
       } else if (parseToken(lltok::exclaim, "Expected '!' here") ||
@@ -5569,7 +5560,7 @@ bool LLParser::parseDILabel(MDNode *&Result, bool IsDistinct) {
 
 /// parseDIExpression:
 ///   ::= !DIExpression(0, 7, -1)
-bool LLParser::parseDIExpression(MDNode *&Result, bool IsDistinct) {
+bool LLParser::parseDIExpression(Metadata *&Result) {
   assert(Lex.getKind() == lltok::MetadataVar && "Expected metadata type name");
   Lex.Lex();
 
@@ -5611,7 +5602,7 @@ bool LLParser::parseDIExpression(MDNode *&Result, bool IsDistinct) {
   if (parseToken(lltok::rparen, "expected ')' here"))
     return true;
 
-  Result = GET_OR_DISTINCT(DIExpression, (Context, Elements));
+  Result = DIExpression::get(Context, Elements);
   return false;
 }
 
@@ -5760,6 +5751,13 @@ bool LLParser::parseMetadata(Metadata *&MD, PerFunctionState *PFS) {
       MD = AL;
       return false;
     }
+    else if (Lex.getStrVal() == "DIExpression") {
+      Metadata *Expr;
+      if (parseDIExpression(Expr))
+        return true;
+      MD = Expr;
+      return false;
+    }
     MDNode *N;
     if (parseSpecializedMDNode(N)) {
       return true;
diff --git a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
index 770eb83af17f9b0..6ee44110d47a145 100644
--- a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
+++ b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
@@ -2145,7 +2145,6 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata(
     if (Record.size() < 1)
       return error("Invalid record");
 
-    IsDistinct = Record[0] & 1;
     uint64_t Version = Record[0] >> 1;
     auto Elts = MutableArrayRef<uint64_t>(Record).slice(1);
 
@@ -2153,7 +2152,7 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata(
     if (Error Err = upgradeDIExpression(Version, Elts, Buffer))
       return Err;
 
-    MetadataList.assignValue(GET_OR_DISTINCT(DIExpression, (Context, Elts)),
+    MetadataList.assignValue(DIExpression::get(Context, Elts),
                              NextMetadataNo);
     NextMetadataNo++;
     break;
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index a5fc267b1883bfe..d2f05597deb5a58 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -355,7 +355,7 @@ class ModuleBitcodeWriter : public ModuleBitcodeWriterBase {
   void writeDILabel(const DILabel *N,
                     SmallVectorImpl<uint64_t> &Record, unsigned Abbrev);
   void writeDIExpression(const DIExpression *N,
-                         SmallVectorImpl<uint64_t> &Record, unsigned Abbrev);
+                         SmallVectorImpl<uint64_t> &Record);
   void writeDIGlobalVariableExpression(const DIGlobalVariableExpression *N,
                                        SmallVectorImpl<uint64_t> &Record,
                                        unsigned Abbrev);
@@ -2138,14 +2138,13 @@ void ModuleBitcodeWriter::writeDILabel(
 }
 
 void ModuleBitcodeWriter::writeDIExpression(const DIExpression *N,
-                                            SmallVectorImpl<uint64_t> &Record,
-                                            unsigned Abbrev) {
+                                            SmallVectorImpl<uint64_t> &Record) {
   Record.reserve(N->getElements().size() + 1);
   const uint64_t Version = 3 << 1;
-  Record.push_back((uint64_t)N->isDistinct() | Version);
+  Record.push_back(Version);
   Record.append(N->elements_begin(), N->elements_end());
 
-  Stream.EmitRecord(bitc::METADATA_EXPRESSION, Record, Abbrev);
+  Stream.EmitRecord(bitc::METADATA_EXPRESSION, Record);
   Record.clear();
 }
 
@@ -2305,6 +2304,10 @@ void ModuleBitcodeWriter::writeMetadataRecords(
       writeDIArgList(AL, Record);
       continue;
     }
+    if (auto *AL = dyn_cast<DIExpression>(MD)) {
+      writeDIExpression(AL, Record);
+      continue;
+    }
     writeValueAsMetadata(cast<ValueAsMetadata>(MD), Record);
   }
 }
diff --git a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
index a5827c26c04f48b..21aebf0166e5684 100644
--- a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
@@ -48,7 +48,7 @@ MachineInstrBuilder MachineIRBuilder::insertInstr(MachineInstrBuilder MIB) {
 
 MachineInstrBuilder
 MachineIRBuilder::buildDirectDbgValue(Register Reg, const MDNode *Variable,
-                                      const MDNode *Expr) {
+                                      const Metadata *Expr) {
   assert(isa<DILocalVariable>(Variable) && "not a variable");
   assert(cast<DIExpression>(Expr)->isValid() && "not an expression");
   assert(
@@ -61,7 +61,7 @@ MachineIRBuilder::buildDirectDbgValue(Register Reg, const MDNode *Variable,
 
 MachineInstrBuilder
 MachineIRBuilder::buildIndirectDbgValue(Register Reg, const MDNode *Variable,
-                                        const MDNode *Expr) {
+                                        const Metadata *Expr) {
   assert(isa<DILocalVariable>(Variable) && "not a variable");
   assert(cast<DIExpression>(Expr)->isValid() && "not an expression");
   assert(
@@ -74,7 +74,7 @@ MachineIRBuilder::buildIndirectDbgValue(Register Reg, const MDNode *Variable,
 
 MachineInstrBuilder MachineIRBuilder::buildFIDbgValue(int FI,
                                                       const MDNode *Variable,
-                                                      const MDNode *Expr) {
+                                                      const Metadata *Expr) {
   assert(isa<DILocalVariable>(Variable) && "not a variable");
   assert(cast<DIExpression>(Expr)->isValid() && "not an expression");
   assert(
@@ -89,7 +89,7 @@ MachineInstrBuilder MachineIRBuilder::buildFIDbgValue(int FI,
 
 MachineInstrBuilder MachineIRBuilder::buildConstDbgValue(const Constant &C,
                                                          const MDNode *Variable,
-                                                         const MDNode *Expr) {
+                                                         const Metadata *Expr) {
   assert(isa<DILocalVariable>(Variable) && "not a variable");
   assert(cast<DIExpression>(Expr)->isValid() && "not an expression");
   assert(
diff --git a/llvm/lib/CodeGen/MIRParser/MIParser.cpp b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
index ede4291fe26dc95..d545e21c3d12ace 100644
--- a/llvm/lib/CodeGen/MIRParser/MIParser.cpp
+++ b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
@@ -472,7 +472,7 @@ class MIParser {
   bool parseExternalSymbolOperand(MachineOperand &Dest);
   bool parseMCSymbolOperand(MachineOperand &Dest);
   [[nodiscard]] bool parseMDNode(MDNode *&Node);
-  bool parseDIExpression(MDNode *&Expr);
+  bool parseDIExpression(Metadata *&Expr);
   bool parseDILocation(MDNode *&Expr);
   bool parseMetadataOperand(MachineOperand &Dest);
   bool parseCFIOffset(int &Offset);
@@ -1269,9 +1269,6 @@ bool MIParser::parseStandaloneMDNode(MDNode *&Node) {
   if (Token.is(MIToken::exclaim)) {
     if (parseMDNode(Node))
       return true;
-  } else if (Token.is(MIToken::md_diexpr)) {
-    if (parseDIExpression(Node))
-      return true;
   } else if (Token.is(MIToken::md_dilocation)) {
     if (parseDILocation(Node))
       return true;
@@ -2292,7 +2289,7 @@ bool MIParser::parseMDNode(MDNode *&Node) {
   return false;
 }
 
-bool MIParser::parseDIExpression(MDNode *&Expr) {
+bool MIParser::parseDIExpression(Metadata *&Expr) {
   assert(Token.is(MIToken::md_diexpr));
   lex();
 
@@ -2442,15 +2439,15 @@ bool MIParser::parseDILocation(MDNode *&Loc) {
 }
 
 bool MIParser::parseMetadataOperand(MachineOperand &Dest) {
-  MDNode *Node = nullptr;
+    Metadata *Meta = nullptr;
   if (Token.is(MIToken::exclaim)) {
-    if (parseMDNode(Node))
+     if (parseMDNode(reinterpret_cast<MDNode *&>(Meta)))
       return true;
   } else if (Token.is(MIToken::md_diexpr)) {
-    if (parseDIExpression(Node))
+    if (parseDIExpression(Meta))
       return true;
   }
-  Dest = MachineOperand::CreateMetadata(Node);
+  Dest = MachineOperand::CreateMetadata(reinterpret_cast<MDNode *>(Meta));
   return false;
 }
 
diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp
index 27eae372f8ad764..740edf06eb2b205 100644
--- a/llvm/lib/CodeGen/MachineInstr.cpp
+++ b/llvm/lib/CodeGen/MachineInstr.cpp
@@ -2153,7 +2153,7 @@ void MachineInstr::emitError(StringRef Msg) const {
 MachineInstrBuilder llvm::BuildMI(MachineFunction &MF, const DebugLoc &DL,
                                   const MCInstrDesc &MCID, bool IsIndirect,
                                   Register Reg, const MDNode *Variable,
-                                  const MDNode *Expr) {
+                                  const Metadata *Expr) {
   assert(isa<DILocalVariable>(Variable) && "not a variable");
   assert(cast<DIExpression>(Expr)->isValid() && "not an expression");
   assert(cast<DILocalVariable>(Variable)->isValidLocationForIntrinsic(DL) &&
@@ -2169,7 +2169,7 @@ MachineInstrBuilder llvm::Buil...
[truncated]

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 24, 2024

@llvm/pr-subscribers-backend-webassembly

Author: Shivam (phyBrackets)

Changes

DIExpression no longer inherits from MDNode. This change was motivated by the need to ensure that DIExpression instances have a unique storage type and there was a bit discussion over on discourse .


Patch is 30.18 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/79335.diff

20 Files Affected:

  • (modified) llvm/include/llvm/AsmParser/LLParser.h (+1)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h (+4-4)
  • (modified) llvm/include/llvm/CodeGen/MachineInstrBuilder.h (+15-4)
  • (modified) llvm/include/llvm/CodeGen/MachineOperand.h (+14-2)
  • (modified) llvm/include/llvm/IR/DebugInfoMetadata.h (+11-15)
  • (modified) llvm/include/llvm/IR/Metadata.def (+1-1)
  • (modified) llvm/lib/AsmParser/LLParser.cpp (+10-12)
  • (modified) llvm/lib/Bitcode/Reader/MetadataLoader.cpp (+1-2)
  • (modified) llvm/lib/Bitcode/Writer/BitcodeWriter.cpp (+8-5)
  • (modified) llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp (+4-4)
  • (modified) llvm/lib/CodeGen/MIRParser/MIParser.cpp (+6-9)
  • (modified) llvm/lib/CodeGen/MachineInstr.cpp (+4-4)
  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp (+1-1)
  • (modified) llvm/lib/IR/DIBuilder.cpp (-2)
  • (modified) llvm/lib/IR/DebugInfoMetadata.cpp (+14-5)
  • (modified) llvm/lib/IR/LLVMContextImpl.h (+46)
  • (modified) llvm/lib/IR/Verifier.cpp (+1)
  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp (+1-1)
  • (modified) llvm/lib/Transforms/Utils/ValueMapper.cpp (+10-2)
  • (modified) llvm/unittests/IR/MetadataTest.cpp (-3)
diff --git a/llvm/include/llvm/AsmParser/LLParser.h b/llvm/include/llvm/AsmParser/LLParser.h
index cf358c384f52033..af4423cb4a7ca15 100644
--- a/llvm/include/llvm/AsmParser/LLParser.h
+++ b/llvm/include/llvm/AsmParser/LLParser.h
@@ -579,6 +579,7 @@ namespace llvm {
     bool parseValueAsMetadata(Metadata *&MD, const Twine &TypeMsg,
                               PerFunctionState *PFS);
     bool parseDIArgList(Metadata *&MD, PerFunctionState *PFS);
+    bool parseDIExpression(Metadata *&MD);
     bool parseMetadata(Metadata *&MD, PerFunctionState *PFS);
     bool parseMDTuple(MDNode *&MD, bool IsDistinct = false);
     bool parseMDNode(MDNode *&N);
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
index 1387a0a37561c4b..2691188d9cffc17 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
@@ -410,26 +410,26 @@ class MachineIRBuilder {
   /// Build and insert a DBG_VALUE instruction expressing the fact that the
   /// associated \p Variable lives in \p Reg (suitably modified by \p Expr).
   MachineInstrBuilder buildDirectDbgValue(Register Reg, const MDNode *Variable,
-                                          const MDNode *Expr);
+                                          const Metadata *Expr);
 
   /// Build and insert a DBG_VALUE instruction expressing the fact that the
   /// associated \p Variable lives in memory at \p Reg (suitably modified by \p
   /// Expr).
   MachineInstrBuilder buildIndirectDbgValue(Register Reg,
                                             const MDNode *Variable,
-                                            const MDNode *Expr);
+                                            const Metadata *Expr);
 
   /// Build and insert a DBG_VALUE instruction expressing the fact that the
   /// associated \p Variable lives in the stack slot specified by \p FI
   /// (suitably modified by \p Expr).
   MachineInstrBuilder buildFIDbgValue(int FI, const MDNode *Variable,
-                                      const MDNode *Expr);
+                                      const Metadata *Expr);
 
   /// Build and insert a DBG_VALUE instructions specifying that \p Variable is
   /// given by \p C (suitably modified by \p Expr).
   MachineInstrBuilder buildConstDbgValue(const Constant &C,
                                          const MDNode *Variable,
-                                         const MDNode *Expr);
+                                         const Metadata *Expr);
 
   /// Build and insert a DBG_LABEL instructions specifying that \p Label is
   /// given. Convert "llvm.dbg.label Label" to "DBG_LABEL Label".
diff --git a/llvm/include/llvm/CodeGen/MachineInstrBuilder.h b/llvm/include/llvm/CodeGen/MachineInstrBuilder.h
index 954d8e6770a294f..c0c2a4ea4c0c23a 100644
--- a/llvm/include/llvm/CodeGen/MachineInstrBuilder.h
+++ b/llvm/include/llvm/CodeGen/MachineInstrBuilder.h
@@ -245,6 +245,17 @@ class MachineInstrBuilder {
     return *this;
   }
 
+  const MachineInstrBuilder &addMetadata(const Metadata *MD) const {
+    MI->addOperand(*MF, MachineOperand::CreateMetadata(MD));
+    assert((MI->isDebugValueLike() ? static_cast<bool>(MI->getDebugVariable())
+                                   : true) &&
+           "first MDNode argument of a DBG_VALUE not a variable");
+    assert((MI->isDebugLabel() ? static_cast<bool>(MI->getDebugLabel())
+                               : true) &&
+           "first MDNode argument of a DBG_LABEL not a label");
+    return *this;
+  }
+
   const MachineInstrBuilder &addCFIIndex(unsigned CFIIndex) const {
     MI->addOperand(*MF, MachineOperand::CreateCFIIndex(CFIIndex));
     return *this;
@@ -488,14 +499,14 @@ inline MachineInstrBuilder BuildMI(MachineBasicBlock *BB,
 MachineInstrBuilder BuildMI(MachineFunction &MF, const DebugLoc &DL,
                             const MCInstrDesc &MCID, bool IsIndirect,
                             Register Reg, const MDNode *Variable,
-                            const MDNode *Expr);
+                            const Metadata *Expr);
 
 /// This version of the builder builds a DBG_VALUE or DBG_VALUE_LIST intrinsic
 /// for a MachineOperand.
 MachineInstrBuilder BuildMI(MachineFunction &MF, const DebugLoc &DL,
                             const MCInstrDesc &MCID, bool IsIndirect,
                             ArrayRef<MachineOperand> MOs,
-                            const MDNode *Variable, const MDNode *Expr);
+                            const MDNode *Variable, const Metadata *Expr);
 
 /// This version of the builder builds a DBG_VALUE intrinsic
 /// for either a value in a register or a register-indirect
@@ -504,7 +515,7 @@ MachineInstrBuilder BuildMI(MachineBasicBlock &BB,
                             MachineBasicBlock::iterator I, const DebugLoc &DL,
                             const MCInstrDesc &MCID, bool IsIndirect,
                             Register Reg, const MDNode *Variable,
-                            const MDNode *Expr);
+                            const Metadata *Expr);
 
 /// This version of the builder builds a DBG_VALUE, DBG_INSTR_REF, or
 /// DBG_VALUE_LIST intrinsic for a machine operand and inserts it at position I.
@@ -512,7 +523,7 @@ MachineInstrBuilder BuildMI(MachineBasicBlock &BB,
                             MachineBasicBlock::iterator I, const DebugLoc &DL,
                             const MCInstrDesc &MCID, bool IsIndirect,
                             ArrayRef<MachineOperand> MOs,
-                            const MDNode *Variable, const MDNode *Expr);
+                            const MDNode *Variable, const Metadata *Expr);
 
 /// Clone a DBG_VALUE whose value has been spilled to FrameIndex.
 MachineInstr *buildDbgValueForSpill(MachineBasicBlock &BB,
diff --git a/llvm/include/llvm/CodeGen/MachineOperand.h b/llvm/include/llvm/CodeGen/MachineOperand.h
index 63a172134538c42..9d5532c7d812ec1 100644
--- a/llvm/include/llvm/CodeGen/MachineOperand.h
+++ b/llvm/include/llvm/CodeGen/MachineOperand.h
@@ -31,6 +31,7 @@ class MachineInstr;
 class MachineRegisterInfo;
 class MCCFIInstruction;
 class MDNode;
+class Metadata;
 class ModuleSlotTracker;
 class TargetIntrinsicInfo;
 class TargetRegisterInfo;
@@ -173,6 +174,7 @@ class MachineOperand {
     int64_t ImmVal;          // For MO_Immediate.
     const uint32_t *RegMask; // For MO_RegisterMask and MO_RegisterLiveOut.
     const MDNode *MD;        // For MO_Metadata.
+    const Metadata* Expr;
     MCSymbol *Sym;           // For MO_MCSymbol.
     unsigned CFIIndex;       // For MO_CFI.
     Intrinsic::ID IntrinsicID; // For MO_IntrinsicID.
@@ -677,6 +679,11 @@ class MachineOperand {
     return Contents.MD;
   }
 
+  const Metadata *getMetadataDI() const {
+    assert(isMetadata() && "Wrong MachineOperand accessor");
+    return Contents.Expr;
+  }
+
   //===--------------------------------------------------------------------===//
   // Mutators for various operand types.
   //===--------------------------------------------------------------------===//
@@ -710,9 +717,9 @@ class MachineOperand {
     Contents.OffsetedInfo.Val.Index = Idx;
   }
 
-  void setMetadata(const MDNode *MD) {
+  void setMetadata(const Metadata *MD) {
     assert(isMetadata() && "Wrong MachineOperand mutator");
-    Contents.MD = MD;
+    Contents.Expr = MD;
   }
 
   void setInstrRefInstrIndex(unsigned InstrIdx) {
@@ -946,6 +953,11 @@ class MachineOperand {
     Op.Contents.MD = Meta;
     return Op;
   }
+  static MachineOperand CreateMetadata(const Metadata *Meta) {
+    MachineOperand Op(MachineOperand::MO_Metadata);
+    Op.Contents.Expr = Meta;
+    return Op;
+  }
 
   static MachineOperand CreateMCSymbol(MCSymbol *Sym,
                                        unsigned TargetFlags = 0) {
diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h
index 156f6eb49253de7..207deed8a78dd97 100644
--- a/llvm/include/llvm/IR/DebugInfoMetadata.h
+++ b/llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -2656,31 +2656,27 @@ class DIVariable : public DINode {
 /// DW_OP_stack_value) is the constant variable value.
 ///
 /// TODO: Co-allocate the expression elements.
-/// TODO: Separate from MDNode, or otherwise drop Distinct and Temporary
-/// storage types.
-class DIExpression : public MDNode {
+class DIExpression : public Metadata, ReplaceableMetadataImpl {
+  friend class ReplaceableMetadataImpl;
   friend class LLVMContextImpl;
-  friend class MDNode;
 
   std::vector<uint64_t> Elements;
 
-  DIExpression(LLVMContext &C, StorageType Storage, ArrayRef<uint64_t> Elements)
-      : MDNode(C, DIExpressionKind, Storage, std::nullopt),
+  DIExpression(LLVMContext &C, ArrayRef<uint64_t> Elements)
+      : Metadata(DIExpressionKind, Uniqued), ReplaceableMetadataImpl(C),
         Elements(Elements.begin(), Elements.end()) {}
   ~DIExpression() = default;
 
   static DIExpression *getImpl(LLVMContext &Context,
-                               ArrayRef<uint64_t> Elements, StorageType Storage,
-                               bool ShouldCreate = true);
-
-  TempDIExpression cloneImpl() const {
-    return getTemporary(getContext(), getElements());
-  }
+                               ArrayRef<uint64_t> Elements);
 
 public:
-  DEFINE_MDNODE_GET(DIExpression, (ArrayRef<uint64_t> Elements), (Elements))
-
-  TempDIExpression clone() const { return cloneImpl(); }
+    LLVMContext &getContext() const {
+        return ReplaceableMetadataImpl::getContext();
+    }
+    static inline DIExpression *get(LLVMContext &Context, ArrayRef<uint64_t> Elements) {
+        return getImpl(Context, Elements);
+    }
 
   ArrayRef<uint64_t> getElements() const { return Elements; }
 
diff --git a/llvm/include/llvm/IR/Metadata.def b/llvm/include/llvm/IR/Metadata.def
index a3cfb9ad6e3e785..0edbe931c75ed8e 100644
--- a/llvm/include/llvm/IR/Metadata.def
+++ b/llvm/include/llvm/IR/Metadata.def
@@ -81,7 +81,7 @@ HANDLE_METADATA_LEAF(DIArgList)
 HANDLE_MDNODE_BRANCH(MDNode)
 HANDLE_MDNODE_LEAF_UNIQUABLE(MDTuple)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DILocation)
-HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIExpression)
+HANDLE_METADATA_LEAF(DIExpression)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIGlobalVariableExpression)
 HANDLE_SPECIALIZED_MDNODE_BRANCH(DINode)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(GenericDINode)
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index d6c5993797de111..e7b06f8c5fef702 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -892,16 +892,7 @@ bool LLParser::parseNamedMetadata() {
   if (Lex.getKind() != lltok::rbrace)
     do {
       MDNode *N = nullptr;
-      // parse DIExpressions inline as a special case. They are still MDNodes,
-      // so they can still appear in named metadata. Remove this logic if they
-      // become plain Metadata.
-      if (Lex.getKind() == lltok::MetadataVar &&
-          Lex.getStrVal() == "DIExpression") {
-        if (parseDIExpression(N, /*IsDistinct=*/false))
-          return true;
-        // DIArgLists should only appear inline in a function, as they may
-        // contain LocalAsMetadata arguments which require a function context.
-      } else if (Lex.getKind() == lltok::MetadataVar &&
+       if (Lex.getKind() == lltok::MetadataVar &&
                  Lex.getStrVal() == "DIArgList") {
         return tokError("found DIArgList outside of function");
       } else if (parseToken(lltok::exclaim, "Expected '!' here") ||
@@ -5569,7 +5560,7 @@ bool LLParser::parseDILabel(MDNode *&Result, bool IsDistinct) {
 
 /// parseDIExpression:
 ///   ::= !DIExpression(0, 7, -1)
-bool LLParser::parseDIExpression(MDNode *&Result, bool IsDistinct) {
+bool LLParser::parseDIExpression(Metadata *&Result) {
   assert(Lex.getKind() == lltok::MetadataVar && "Expected metadata type name");
   Lex.Lex();
 
@@ -5611,7 +5602,7 @@ bool LLParser::parseDIExpression(MDNode *&Result, bool IsDistinct) {
   if (parseToken(lltok::rparen, "expected ')' here"))
     return true;
 
-  Result = GET_OR_DISTINCT(DIExpression, (Context, Elements));
+  Result = DIExpression::get(Context, Elements);
   return false;
 }
 
@@ -5760,6 +5751,13 @@ bool LLParser::parseMetadata(Metadata *&MD, PerFunctionState *PFS) {
       MD = AL;
       return false;
     }
+    else if (Lex.getStrVal() == "DIExpression") {
+      Metadata *Expr;
+      if (parseDIExpression(Expr))
+        return true;
+      MD = Expr;
+      return false;
+    }
     MDNode *N;
     if (parseSpecializedMDNode(N)) {
       return true;
diff --git a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
index 770eb83af17f9b0..6ee44110d47a145 100644
--- a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
+++ b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
@@ -2145,7 +2145,6 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata(
     if (Record.size() < 1)
       return error("Invalid record");
 
-    IsDistinct = Record[0] & 1;
     uint64_t Version = Record[0] >> 1;
     auto Elts = MutableArrayRef<uint64_t>(Record).slice(1);
 
@@ -2153,7 +2152,7 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata(
     if (Error Err = upgradeDIExpression(Version, Elts, Buffer))
       return Err;
 
-    MetadataList.assignValue(GET_OR_DISTINCT(DIExpression, (Context, Elts)),
+    MetadataList.assignValue(DIExpression::get(Context, Elts),
                              NextMetadataNo);
     NextMetadataNo++;
     break;
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index a5fc267b1883bfe..d2f05597deb5a58 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -355,7 +355,7 @@ class ModuleBitcodeWriter : public ModuleBitcodeWriterBase {
   void writeDILabel(const DILabel *N,
                     SmallVectorImpl<uint64_t> &Record, unsigned Abbrev);
   void writeDIExpression(const DIExpression *N,
-                         SmallVectorImpl<uint64_t> &Record, unsigned Abbrev);
+                         SmallVectorImpl<uint64_t> &Record);
   void writeDIGlobalVariableExpression(const DIGlobalVariableExpression *N,
                                        SmallVectorImpl<uint64_t> &Record,
                                        unsigned Abbrev);
@@ -2138,14 +2138,13 @@ void ModuleBitcodeWriter::writeDILabel(
 }
 
 void ModuleBitcodeWriter::writeDIExpression(const DIExpression *N,
-                                            SmallVectorImpl<uint64_t> &Record,
-                                            unsigned Abbrev) {
+                                            SmallVectorImpl<uint64_t> &Record) {
   Record.reserve(N->getElements().size() + 1);
   const uint64_t Version = 3 << 1;
-  Record.push_back((uint64_t)N->isDistinct() | Version);
+  Record.push_back(Version);
   Record.append(N->elements_begin(), N->elements_end());
 
-  Stream.EmitRecord(bitc::METADATA_EXPRESSION, Record, Abbrev);
+  Stream.EmitRecord(bitc::METADATA_EXPRESSION, Record);
   Record.clear();
 }
 
@@ -2305,6 +2304,10 @@ void ModuleBitcodeWriter::writeMetadataRecords(
       writeDIArgList(AL, Record);
       continue;
     }
+    if (auto *AL = dyn_cast<DIExpression>(MD)) {
+      writeDIExpression(AL, Record);
+      continue;
+    }
     writeValueAsMetadata(cast<ValueAsMetadata>(MD), Record);
   }
 }
diff --git a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
index a5827c26c04f48b..21aebf0166e5684 100644
--- a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
@@ -48,7 +48,7 @@ MachineInstrBuilder MachineIRBuilder::insertInstr(MachineInstrBuilder MIB) {
 
 MachineInstrBuilder
 MachineIRBuilder::buildDirectDbgValue(Register Reg, const MDNode *Variable,
-                                      const MDNode *Expr) {
+                                      const Metadata *Expr) {
   assert(isa<DILocalVariable>(Variable) && "not a variable");
   assert(cast<DIExpression>(Expr)->isValid() && "not an expression");
   assert(
@@ -61,7 +61,7 @@ MachineIRBuilder::buildDirectDbgValue(Register Reg, const MDNode *Variable,
 
 MachineInstrBuilder
 MachineIRBuilder::buildIndirectDbgValue(Register Reg, const MDNode *Variable,
-                                        const MDNode *Expr) {
+                                        const Metadata *Expr) {
   assert(isa<DILocalVariable>(Variable) && "not a variable");
   assert(cast<DIExpression>(Expr)->isValid() && "not an expression");
   assert(
@@ -74,7 +74,7 @@ MachineIRBuilder::buildIndirectDbgValue(Register Reg, const MDNode *Variable,
 
 MachineInstrBuilder MachineIRBuilder::buildFIDbgValue(int FI,
                                                       const MDNode *Variable,
-                                                      const MDNode *Expr) {
+                                                      const Metadata *Expr) {
   assert(isa<DILocalVariable>(Variable) && "not a variable");
   assert(cast<DIExpression>(Expr)->isValid() && "not an expression");
   assert(
@@ -89,7 +89,7 @@ MachineInstrBuilder MachineIRBuilder::buildFIDbgValue(int FI,
 
 MachineInstrBuilder MachineIRBuilder::buildConstDbgValue(const Constant &C,
                                                          const MDNode *Variable,
-                                                         const MDNode *Expr) {
+                                                         const Metadata *Expr) {
   assert(isa<DILocalVariable>(Variable) && "not a variable");
   assert(cast<DIExpression>(Expr)->isValid() && "not an expression");
   assert(
diff --git a/llvm/lib/CodeGen/MIRParser/MIParser.cpp b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
index ede4291fe26dc95..d545e21c3d12ace 100644
--- a/llvm/lib/CodeGen/MIRParser/MIParser.cpp
+++ b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
@@ -472,7 +472,7 @@ class MIParser {
   bool parseExternalSymbolOperand(MachineOperand &Dest);
   bool parseMCSymbolOperand(MachineOperand &Dest);
   [[nodiscard]] bool parseMDNode(MDNode *&Node);
-  bool parseDIExpression(MDNode *&Expr);
+  bool parseDIExpression(Metadata *&Expr);
   bool parseDILocation(MDNode *&Expr);
   bool parseMetadataOperand(MachineOperand &Dest);
   bool parseCFIOffset(int &Offset);
@@ -1269,9 +1269,6 @@ bool MIParser::parseStandaloneMDNode(MDNode *&Node) {
   if (Token.is(MIToken::exclaim)) {
     if (parseMDNode(Node))
       return true;
-  } else if (Token.is(MIToken::md_diexpr)) {
-    if (parseDIExpression(Node))
-      return true;
   } else if (Token.is(MIToken::md_dilocation)) {
     if (parseDILocation(Node))
       return true;
@@ -2292,7 +2289,7 @@ bool MIParser::parseMDNode(MDNode *&Node) {
   return false;
 }
 
-bool MIParser::parseDIExpression(MDNode *&Expr) {
+bool MIParser::parseDIExpression(Metadata *&Expr) {
   assert(Token.is(MIToken::md_diexpr));
   lex();
 
@@ -2442,15 +2439,15 @@ bool MIParser::parseDILocation(MDNode *&Loc) {
 }
 
 bool MIParser::parseMetadataOperand(MachineOperand &Dest) {
-  MDNode *Node = nullptr;
+    Metadata *Meta = nullptr;
   if (Token.is(MIToken::exclaim)) {
-    if (parseMDNode(Node))
+     if (parseMDNode(reinterpret_cast<MDNode *&>(Meta)))
       return true;
   } else if (Token.is(MIToken::md_diexpr)) {
-    if (parseDIExpression(Node))
+    if (parseDIExpression(Meta))
       return true;
   }
-  Dest = MachineOperand::CreateMetadata(Node);
+  Dest = MachineOperand::CreateMetadata(reinterpret_cast<MDNode *>(Meta));
   return false;
 }
 
diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp
index 27eae372f8ad764..740edf06eb2b205 100644
--- a/llvm/lib/CodeGen/MachineInstr.cpp
+++ b/llvm/lib/CodeGen/MachineInstr.cpp
@@ -2153,7 +2153,7 @@ void MachineInstr::emitError(StringRef Msg) const {
 MachineInstrBuilder llvm::BuildMI(MachineFunction &MF, const DebugLoc &DL,
                                   const MCInstrDesc &MCID, bool IsIndirect,
                                   Register Reg, const MDNode *Variable,
-                                  const MDNode *Expr) {
+                                  const Metadata *Expr) {
   assert(isa<DILocalVariable>(Variable) && "not a variable");
   assert(cast<DIExpression>(Expr)->isValid() && "not an expression");
   assert(cast<DILocalVariable>(Variable)->isValidLocationForIntrinsic(DL) &&
@@ -2169,7 +2169,7 @@ MachineInstrBuilder llvm::Buil...
[truncated]

Copy link

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 56602a48c735a1c906a9ec4e03a64fd9c937def3 b577ea3a85367df9af067619b01ee2a0cca8894d -- llvm/include/llvm/AsmParser/LLParser.h llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h llvm/include/llvm/CodeGen/MachineInstrBuilder.h llvm/include/llvm/CodeGen/MachineOperand.h llvm/include/llvm/IR/DebugInfoMetadata.h llvm/lib/AsmParser/LLParser.cpp llvm/lib/Bitcode/Reader/MetadataLoader.cpp llvm/lib/Bitcode/Writer/BitcodeWriter.cpp llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp llvm/lib/CodeGen/MIRParser/MIParser.cpp llvm/lib/CodeGen/MachineInstr.cpp llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp llvm/lib/IR/DIBuilder.cpp llvm/lib/IR/DebugInfoMetadata.cpp llvm/lib/IR/LLVMContextImpl.h llvm/lib/IR/Verifier.cpp llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp llvm/lib/Transforms/Utils/ValueMapper.cpp llvm/unittests/IR/MetadataTest.cpp
View the diff from clang-format here.
diff --git a/llvm/include/llvm/CodeGen/MachineInstrBuilder.h b/llvm/include/llvm/CodeGen/MachineInstrBuilder.h
index c0c2a4ea4c..b62d43c8fe 100644
--- a/llvm/include/llvm/CodeGen/MachineInstrBuilder.h
+++ b/llvm/include/llvm/CodeGen/MachineInstrBuilder.h
@@ -250,9 +250,9 @@ public:
     assert((MI->isDebugValueLike() ? static_cast<bool>(MI->getDebugVariable())
                                    : true) &&
            "first MDNode argument of a DBG_VALUE not a variable");
-    assert((MI->isDebugLabel() ? static_cast<bool>(MI->getDebugLabel())
-                               : true) &&
-           "first MDNode argument of a DBG_LABEL not a label");
+    assert(
+        (MI->isDebugLabel() ? static_cast<bool>(MI->getDebugLabel()) : true) &&
+        "first MDNode argument of a DBG_LABEL not a label");
     return *this;
   }
 
diff --git a/llvm/include/llvm/CodeGen/MachineOperand.h b/llvm/include/llvm/CodeGen/MachineOperand.h
index 9d5532c7d8..e5e80ebf11 100644
--- a/llvm/include/llvm/CodeGen/MachineOperand.h
+++ b/llvm/include/llvm/CodeGen/MachineOperand.h
@@ -174,7 +174,7 @@ private:
     int64_t ImmVal;          // For MO_Immediate.
     const uint32_t *RegMask; // For MO_RegisterMask and MO_RegisterLiveOut.
     const MDNode *MD;        // For MO_Metadata.
-    const Metadata* Expr;
+    const Metadata *Expr;
     MCSymbol *Sym;           // For MO_MCSymbol.
     unsigned CFIIndex;       // For MO_CFI.
     Intrinsic::ID IntrinsicID; // For MO_IntrinsicID.
diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h
index 207deed8a7..ce4d0bc1f7 100644
--- a/llvm/include/llvm/IR/DebugInfoMetadata.h
+++ b/llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -2671,12 +2671,13 @@ class DIExpression : public Metadata, ReplaceableMetadataImpl {
                                ArrayRef<uint64_t> Elements);
 
 public:
-    LLVMContext &getContext() const {
-        return ReplaceableMetadataImpl::getContext();
-    }
-    static inline DIExpression *get(LLVMContext &Context, ArrayRef<uint64_t> Elements) {
-        return getImpl(Context, Elements);
-    }
+  LLVMContext &getContext() const {
+    return ReplaceableMetadataImpl::getContext();
+  }
+  static inline DIExpression *get(LLVMContext &Context,
+                                  ArrayRef<uint64_t> Elements) {
+    return getImpl(Context, Elements);
+  }
 
   ArrayRef<uint64_t> getElements() const { return Elements; }
 
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index e7b06f8c5f..939171edeb 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -892,8 +892,8 @@ bool LLParser::parseNamedMetadata() {
   if (Lex.getKind() != lltok::rbrace)
     do {
       MDNode *N = nullptr;
-       if (Lex.getKind() == lltok::MetadataVar &&
-                 Lex.getStrVal() == "DIArgList") {
+      if (Lex.getKind() == lltok::MetadataVar &&
+          Lex.getStrVal() == "DIArgList") {
         return tokError("found DIArgList outside of function");
       } else if (parseToken(lltok::exclaim, "Expected '!' here") ||
                  parseMDNodeID(N)) {
@@ -5750,8 +5750,7 @@ bool LLParser::parseMetadata(Metadata *&MD, PerFunctionState *PFS) {
         return true;
       MD = AL;
       return false;
-    }
-    else if (Lex.getStrVal() == "DIExpression") {
+    } else if (Lex.getStrVal() == "DIExpression") {
       Metadata *Expr;
       if (parseDIExpression(Expr))
         return true;
diff --git a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
index 6ee44110d4..2a24c3d009 100644
--- a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
+++ b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
@@ -2152,8 +2152,7 @@ Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata(
     if (Error Err = upgradeDIExpression(Version, Elts, Buffer))
       return Err;
 
-    MetadataList.assignValue(DIExpression::get(Context, Elts),
-                             NextMetadataNo);
+    MetadataList.assignValue(DIExpression::get(Context, Elts), NextMetadataNo);
     NextMetadataNo++;
     break;
   }
diff --git a/llvm/lib/CodeGen/MIRParser/MIParser.cpp b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
index d545e21c3d..eb5a77c131 100644
--- a/llvm/lib/CodeGen/MIRParser/MIParser.cpp
+++ b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
@@ -2439,9 +2439,9 @@ bool MIParser::parseDILocation(MDNode *&Loc) {
 }
 
 bool MIParser::parseMetadataOperand(MachineOperand &Dest) {
-    Metadata *Meta = nullptr;
+  Metadata *Meta = nullptr;
   if (Token.is(MIToken::exclaim)) {
-     if (parseMDNode(reinterpret_cast<MDNode *&>(Meta)))
+    if (parseMDNode(reinterpret_cast<MDNode *&>(Meta)))
       return true;
   } else if (Token.is(MIToken::md_diexpr)) {
     if (parseDIExpression(Meta))
diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp
index 740edf06eb..70d293bae4 100644
--- a/llvm/lib/CodeGen/MachineInstr.cpp
+++ b/llvm/lib/CodeGen/MachineInstr.cpp
@@ -2169,7 +2169,8 @@ MachineInstrBuilder llvm::BuildMI(MachineFunction &MF, const DebugLoc &DL,
 MachineInstrBuilder llvm::BuildMI(MachineFunction &MF, const DebugLoc &DL,
                                   const MCInstrDesc &MCID, bool IsIndirect,
                                   ArrayRef<MachineOperand> DebugOps,
-                                  const MDNode *Variable, const Metadata *Expr) {
+                                  const MDNode *Variable,
+                                  const Metadata *Expr) {
   assert(isa<DILocalVariable>(Variable) && "not a variable");
   assert(cast<DIExpression>(Expr)->isValid() && "not an expression");
   assert(cast<DILocalVariable>(Variable)->isValidLocationForIntrinsic(DL) &&
@@ -2200,23 +2201,21 @@ MachineInstrBuilder llvm::BuildMI(MachineFunction &MF, const DebugLoc &DL,
   return MIB;
 }
 
-MachineInstrBuilder llvm::BuildMI(MachineBasicBlock &BB,
-                                  MachineBasicBlock::iterator I,
-                                  const DebugLoc &DL, const MCInstrDesc &MCID,
-                                  bool IsIndirect, Register Reg,
-                                  const MDNode *Variable, const Metadata *Expr) {
+MachineInstrBuilder
+llvm::BuildMI(MachineBasicBlock &BB, MachineBasicBlock::iterator I,
+              const DebugLoc &DL, const MCInstrDesc &MCID, bool IsIndirect,
+              Register Reg, const MDNode *Variable, const Metadata *Expr) {
   MachineFunction &MF = *BB.getParent();
   MachineInstr *MI = BuildMI(MF, DL, MCID, IsIndirect, Reg, Variable, Expr);
   BB.insert(I, MI);
   return MachineInstrBuilder(MF, MI);
 }
 
-MachineInstrBuilder llvm::BuildMI(MachineBasicBlock &BB,
-                                  MachineBasicBlock::iterator I,
-                                  const DebugLoc &DL, const MCInstrDesc &MCID,
-                                  bool IsIndirect,
-                                  ArrayRef<MachineOperand> DebugOps,
-                                  const MDNode *Variable, const Metadata *Expr) {
+MachineInstrBuilder
+llvm::BuildMI(MachineBasicBlock &BB, MachineBasicBlock::iterator I,
+              const DebugLoc &DL, const MCInstrDesc &MCID, bool IsIndirect,
+              ArrayRef<MachineOperand> DebugOps, const MDNode *Variable,
+              const Metadata *Expr) {
   MachineFunction &MF = *BB.getParent();
   MachineInstr *MI =
       BuildMI(MF, DL, MCID, IsIndirect, DebugOps, Variable, Expr);
diff --git a/llvm/lib/IR/DebugInfoMetadata.cpp b/llvm/lib/IR/DebugInfoMetadata.cpp
index d2a039876f..b8663305f7 100644
--- a/llvm/lib/IR/DebugInfoMetadata.cpp
+++ b/llvm/lib/IR/DebugInfoMetadata.cpp
@@ -1351,7 +1351,8 @@ DILabel *DILabel::getImpl(LLVMContext &Context, Metadata *Scope, MDString *Name,
   Metadata *Ops[] = {Scope, Name, File};
   DEFINE_GETIMPL_STORE(DILabel, (Line), Ops);
 }
-DIExpression *DIExpression::getImpl(LLVMContext &Context, ArrayRef<uint64_t> Elements) {
+DIExpression *DIExpression::getImpl(LLVMContext &Context,
+                                    ArrayRef<uint64_t> Elements) {
   auto &DIExpressions = Context.pImpl->DIExpressions;
   DIExpressionKeyInfo Key(Elements);
 
diff --git a/llvm/lib/IR/LLVMContextImpl.h b/llvm/lib/IR/LLVMContextImpl.h
index 37f1471bb5..e14734aef6 100644
--- a/llvm/lib/IR/LLVMContextImpl.h
+++ b/llvm/lib/IR/LLVMContextImpl.h
@@ -1354,7 +1354,8 @@ struct DIExpressionKeyInfo {
   ArrayRef<uint64_t> Elements;
 
   DIExpressionKeyInfo(ArrayRef<uint64_t> Elements) : Elements(Elements) {}
-  DIExpressionKeyInfo(const DIExpression *Expr) : Elements(Expr->getElements()) {}
+  DIExpressionKeyInfo(const DIExpression *Expr)
+      : Elements(Expr->getElements()) {}
 
   bool isKeyOf(const DIExpression *RHS) const {
     return Elements == RHS->getElements();
@@ -1376,9 +1377,7 @@ struct DIExpressionInfo {
     return DenseMapInfo<DIExpression *>::getTombstoneKey();
   }
 
-  static unsigned getHashValue(const KeyTy &Key) {
-    return Key.getHashValue();
-  }
+  static unsigned getHashValue(const KeyTy &Key) { return Key.getHashValue(); }
 
   static unsigned getHashValue(const DIExpression *Expr) {
     return KeyTy(Expr).getHashValue();
diff --git a/llvm/lib/Transforms/Utils/ValueMapper.cpp b/llvm/lib/Transforms/Utils/ValueMapper.cpp
index eddc37d133..5a54e6555a 100644
--- a/llvm/lib/Transforms/Utils/ValueMapper.cpp
+++ b/llvm/lib/Transforms/Utils/ValueMapper.cpp
@@ -615,7 +615,8 @@ std::optional<Metadata *> MDNodeMapper::tryToMapOperand(const Metadata *Op) {
               M.getVM().getMappedMD(Op)) &&
              "Expected Value to be memoized");
     else
-      assert((isa<DIExpression>(Op) || isa<MDString>(Op) || M.getVM().getMappedMD(Op)) &&
+      assert((isa<DIExpression>(Op) || isa<MDString>(Op) ||
+              M.getVM().getMappedMD(Op)) &&
              "Expected result to be memoized");
 #endif
     return *MappedOp;
@@ -811,8 +812,8 @@ void MDNodeMapper::mapNodesInPOT(UniquedGraph &G) {
         // Handle MDNode
         return &G.getFwdReference(*MDN);
       } else if (isa<DIExpression>(Old)) {
-            return Old;
-          }
+        return Old;
+      }
     });
 
     auto *NewN = MDNode::replaceWithUniqued(std::move(ClonedN));
@@ -883,8 +884,8 @@ std::optional<Metadata *> Mapper::mapSimpleMetadata(const Metadata *MD) {
 
   if (isa<MDString>(MD))
     return const_cast<Metadata *>(MD);
-  
-  if(isa<DIExpression>(MD))
+
+  if (isa<DIExpression>(MD))
     return const_cast<Metadata *>(MD);
 
   // This is a module-level metadata.  If nothing at the module level is

Copy link
Contributor

@SLTozer SLTozer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broadly looks good - just got a couple questions inline about implementation.

@@ -173,6 +174,7 @@ class MachineOperand {
int64_t ImmVal; // For MO_Immediate.
const uint32_t *RegMask; // For MO_RegisterMask and MO_RegisterLiveOut.
const MDNode *MD; // For MO_Metadata.
const Metadata* Expr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the MachineOperand implementation, I think it would be better to change the existing field to Metadata* if possible - although we don't lose any space by having the extra field since this is part of a union, we also don't gain much by having one field for Metadata* and one field for MDNode*, since the former can hold the latter without issue (and for most or all of the cases where we use MDNode we need to check type info and cast anyway).

/// TODO: Separate from MDNode, or otherwise drop Distinct and Temporary
/// storage types.
class DIExpression : public MDNode {
class DIExpression : public Metadata, ReplaceableMetadataImpl {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think DIExpression needs to inherit from ReplaceableMetadataImpl - it will never be temporary, and we never need to track its users or replace its uses.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we document why DIExpression is not an MDNode here?

Copy link
Contributor

@SLTozer SLTozer Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that comment would be something along the lines of "DIExpressions do not refer to other metadata and are always uniqued, so do not need to inherit from MDNode."

@phyBrackets
Copy link
Member Author

Thank You Stephen for the review, I'll be pushing the new change and also looking into the tests that are failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants