[IRBuilder] Generalize debug loc handling for arbitrary metadata.

[fhahn]

Summary

This patch extends IRBuilder to allow adding/preserving arbitrary
metadata on created instructions.

Instead of using references to specific metadata nodes (like DebugLoc),
IRbuilder now keeps a vector of (metadata kind, MDNode *) pairs, which
are added to each created instruction.

The patch itself is a NFC and only moves the existing debug location
handling over to the new system. In a follow-up patch it will be used to
preserve !annotation metadata besides !dbg.

The current approach requires iterating over MetadataToCopy to avoid
adding duplicates, but given that the number of metadata kinds to
copy/preserve is going to be very small initially (0, 1 (for !dbg) or 2
(!dbg and !annotation)) that should not matter.

[lebedev.ri]

This comment has been deleted.

llvm/include/llvm/IR/IRBuilder.h

212–219

Don't you have an invalidation issue here?
What happens if we first call CollectMetadataToCopy() on an instruction that had metadata zz,
and then call it again on an instruction that didn't have that metadata? (the instcombine's pattern)

[fhahn]

Update CollectMetadataToCopy to drop metadata kinds that are in MetadataToCopy but not on the source instruction. Also replace LLVMSetInstDebugLocation with LLVMAddInstMetadata.

[lebedev.ri]

llvm/include/llvm/IR/IRBuilder.h

212–219

Actually, my suggestion is still incorrect, because it still doesn't drop metadata
You want something like

void CollectMetadataToCopy(Instruction *Src,
                           ArrayRef<unsigned> MetadataKinds) {
  // First, refresh (either update or delete) all the existing metadata
  for (unsigned K : make_first_range(MetadataToCopy))
    AddOrRemoveMetadataToCopy(K, Src->getMetadata(K));
  // Then, add new metadata (either add or update)
  for (unsigned K : MetadataKinds)
    AddOrRemoveMetadataToCopy(K, Src->getMetadata(K));
}

[fhahn]

llvm/include/llvm/IR/IRBuilder.h

212–219

Yeah, originally I thought of collecting metadata to strictly increase the metadata to copy. But this is not in line with current practices for DebugLoc, where setting an empty DebugLoc removes the DebugLoc. I think in light of that, it makes sense for CollectMetadataToCopy to also remove entries from MetadataTocopy, if they are not present on Src. Also updated the comment

[fhahn]

llvm/include/llvm/IR/IRBuilder.h

212–219

Actually, my suggestion is still incorrect, because it still doesn't drop metadata

Yeah it only drops metadata that's in MetadataKinds, but not on the instruction. Entries that are in MetadataToCopy but not in MetadataKinds are left untouched. From the comment for the function, I would not expect to update the values for any entry in MetadataToCopy but not in MetadataKinds. I guess there are cases where either behavior could be useful.

I am not sure, but I think it might be simpler to just have CollectMetadataToCopy by purely additive (only add or update entries, but do not drop any) and provide a general way to remove entries, once there's a need. Currently this is only needed for debug locations via SetCurrentDebugLocation.

[lebedev.ri]

Ah no, i guess i'm reading this wrong. LG.
I'm not sure about C API changes, they are likely ABI/API breaking and may require more changs, you might want for whoever maintains that to comment.

llvm/include/llvm/IR/IRBuilder.h

234

This should probably be AddMetadataToInst()/SetInstMetadata()/???,
because i have already misread what this does.

[fhahn]

Undo C API breaking changes, will submit them separately. Also changed the name to AddMetadataToInst.

llvm/include/llvm/IR/IRBuilder.h

234

Thanks, that name is much better! Changed.

[fhahn]

rebase

[fhahn]

LLVM-C API changes are in D93454

[nikic]

FYI this has a noticeable compile-time impact: http://llvm-compile-time-tracker.com/compare.php?from=deae7e982a3b08996455e2cdfdc5062bf37895a3&to=01089c876bff43a7cde1cb9b1ef8c128169ec5b4&stat=instructions (including the change that adjusts InstCombine)

Something to keep in mind is that debug metadata is privileged, and a reference to it is stored in the instruction. Fetching the annotation metadata (even if it does not exist, but *some* kind of metadata exists) requires looking up the instruction in the metadata map and scanning the metadata vector.

[fhahn]

In D93400#2461288, @nikic wrote:

FYI this has a noticeable compile-time impact: http://llvm-compile-time-tracker.com/compare.php?from=deae7e982a3b08996455e2cdfdc5062bf37895a3&to=01089c876bff43a7cde1cb9b1ef8c128169ec5b4&stat=instructions (including the change that adjusts InstCombine)

Something to keep in mind is that debug metadata is privileged, and a reference to it is stored in the instruction. Fetching the annotation metadata (even if it does not exist, but *some* kind of metadata exists) requires looking up the instruction in the metadata map and scanning the metadata vector.

Thanks for the heads up. This is definitely something that needs to be addressed. I'll try a few things tomorrow. Surprising to see there's actually a speedup with the ReleaseLTO + g

llvm/include/llvm/IR/IRBuilder.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_IR_IRBUILDER_H
 #define LLVM_IR_IRBUILDER_H
 
 #include "llvm-c/Types.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/None.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Constant.h"
 #include "llvm/IR/ConstantFolder.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
void InsertHelper(Instruction *I, const Twine &Name,
                     BasicBlock::iterator InsertPt) const override {
     IRBuilderDefaultInserter::InsertHelper(I, Name, BB, InsertPt);
     Callback(I);
   }
 };
 
 /// Common base class shared among various IRBuilders.
 class IRBuilderBase {
-  DebugLoc CurDbgLocation;
+  /// Pairs of (metadata kind, MDNode *) that should be added to all newly
+  /// created instructions, like !dbg metadata.
+  SmallVector<std::pair<unsigned, MDNode *>, 2> MetadataToCopy;
+
+  /// Add or update the an entry (Kind, MD) to MetadataToCopy, if \p MD is not
+  /// null. If \p MD is null, remove the entry with \p Kind.
+  void AddOrRemoveMetadataToCopy(unsigned Kind, MDNode *MD) {
+    if (!MD) {
+      erase_if(MetadataToCopy, [Kind](const std::pair<unsigned, MDNode *> &KV) {
+        return KV.first == Kind;
+      });
+      return;
+    }
+
+    for (auto &KV : MetadataToCopy)
+      if (KV.first == Kind) {
+        KV.second = MD;
+        return;
+      }
+
+    MetadataToCopy.emplace_back(Kind, MD);
+  }
 
 protected:
   BasicBlock *BB;
   BasicBlock::iterator InsertPt;
   LLVMContext &Context;
   const IRBuilderFolder &Folder;
   const IRBuilderDefaultInserter &Inserter;
 
IRBuilderBase(LLVMContext &context, const IRBuilderFolder &Folder,
         DefaultOperandBundles(OpBundles) {
     ClearInsertionPoint();
   }
 
   /// Insert and return the specified instruction.
   template<typename InstTy>
   InstTy *Insert(InstTy *I, const Twine &Name = "") const {
     Inserter.InsertHelper(I, Name, BB, InsertPt);
-    SetInstDebugLocation(I);
+    AddMetadataToInst(I);
     return I;
   }
 
   /// No-op overload to handle constants.
   Constant *Insert(Constant *C, const Twine& = "") const {
     return C;
   }
 
public:
   void SetInsertPoint(BasicBlock *TheBB, BasicBlock::iterator IP) {
     BB = TheBB;
     InsertPt = IP;
     if (IP != TheBB->end())
       SetCurrentDebugLocation(IP->getDebugLoc());
   }
 
   /// Set location information used by debugging information.
-  void SetCurrentDebugLocation(DebugLoc L) { CurDbgLocation = std::move(L); }
+  void SetCurrentDebugLocation(DebugLoc L) {
+    AddOrRemoveMetadataToCopy(LLVMContext::MD_dbg, L.getAsMDNode());
+  }
+
+  /// Collect metadata with IDs \p MetadataKinds from \p Src which should be
+  /// added to all created instructions. Entries present in MedataDataToCopy but
+  /// not on \p Src will be dropped from MetadataToCopy.
+  void CollectMetadataToCopy(Instruction *Src,
+                             ArrayRef<unsigned> MetadataKinds) {
+    for (unsigned K : MetadataKinds)
+      AddOrRemoveMetadataToCopy(K, Src->getMetadata(K));
+  }
 
   /// Get location information used by debugging information.
-  const DebugLoc &getCurrentDebugLocation() const { return CurDbgLocation; }
+  DebugLoc getCurrentDebugLocation() const {
+    for (auto &KV : MetadataToCopy)
+      if (KV.first == LLVMContext::MD_dbg)
+        return {cast<DILocation>(KV.second)};
+
+    return {};
+  }
 
   /// If this builder has a current debug location, set it on the
   /// specified instruction.
   void SetInstDebugLocation(Instruction *I) const {
-    if (CurDbgLocation)
-      I->setDebugLoc(CurDbgLocation);
+    for (const auto &KV : MetadataToCopy)
+      if (KV.first == LLVMContext::MD_dbg) {
+        I->setDebugLoc(DebugLoc(KV.second));
+        return;
+      }
+  }
+
+  /// Add all entries in MetadataToCopy to \p I.
+  void AddMetadataToInst(Instruction *I) const {
+    for (auto &KV : MetadataToCopy)
+      I->setMetadata(KV.first, KV.second);
   }
 
   /// Get the return type of the current function that we're emitting
   /// into.
   Type *getCurrentFunctionReturnType() const;
 
   /// InsertPoint - A saved insertion point.
   class InsertPoint {

llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h

#endif
   void clearInsertPoint() { Builder.ClearInsertionPoint(); }
 
   /// Set location information used by debugging information.
   void SetCurrentDebugLocation(DebugLoc L) {
     Builder.SetCurrentDebugLocation(std::move(L));
   }
 
   /// Get location information used by debugging information.
-  const DebugLoc &getCurrentDebugLocation() const {
+  DebugLoc getCurrentDebugLocation() const {
     return Builder.getCurrentDebugLocation();
   }
 
   /// Return true if the specified instruction was inserted by the code
   /// rewriter.  If so, the client should not modify the instruction. Note that
   /// this also includes instructions re-used during expansion.
   bool isInsertedInstruction(Instruction *I) const {
     return InsertedValues.count(I) || InsertedPostIncValues.count(I);

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

if (EnableCodeSinking)
                   Worklist.push(OpI);
             }
           }
         }
       }
 
     // Now that we have an instruction, try combining it to simplify it.
     Builder.SetInsertPoint(I);
-    Builder.SetCurrentDebugLocation(I->getDebugLoc());
+    Builder.CollectMetadataToCopy(I, {LLVMContext::MD_dbg});
 
 #ifndef NDEBUG
     std::string OrigI;
 #endif
     LLVM_DEBUG(raw_string_ostream SS(OrigI); I->print(SS); OrigI = SS.str(););
     LLVM_DEBUG(dbgs() << "IC: Visiting: " << OrigI << '\n');
 
     if (Instruction *Result = visit(*I)) {