[PATCH] D93400: [IRBuilder] Generalize debug loc handling for arbitrary metadata.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:212-219
+  /// Collect metadata with IDs \p MetadataKinds from \p Src which should be
+  /// added to all created instructions.
+  void CollectMetadataToCopy(Instruction *Src,
+                             ArrayRef<unsigned> MetadataKinds) {
+    for (unsigned K : MetadataKinds)
+      if (auto *MD = Src->getMetadata(K))
+        AddOrRemoveMetadataToCopy(K, MD);
----------------
lebedev.ri wrote:
> fhahn wrote:
> > lebedev.ri wrote:
> > > 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)
> > 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
> 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));
>   }
> ```
> 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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93400/new/

https://reviews.llvm.org/D93400