[PATCH] D98668: Support !heapallocsite attachments in StripDebugInfo().

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added inline comments.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:350
+      if (I.hasMetadataOtherThanDebugLoc())
+        I.setMetadata("heapallocsite", nullptr);
     }
----------------
vsk wrote:
> Is it possible to reach this when the instruction has no heapallocsite metadata?
> 
> If so, we might want a narrower change, like:
> ```
> if (auto *MD = I.getMetadata("heapallocsite"))
>   I.setMetadata("heapallocsite", nullptr);
> ```
> 
> Or we might want to guarantee that we drop non-debug metadata, like:
> ```
> I.dropUnknownNonDebugMetadata();
> ```
> 
> Ditto for D98667.
> Is it possible to reach this when the instruction has no heapallocsite metadata?

It is possible, but the code as written is correct. `nullptr` here effectively means "delete if exists".

We could (maybe should?) drop the `if` entirely and always do:
```
I.setMetadata("healallocsite", nullptr);
```
since Instruction::setMetadata has an early return for exactly the checked condition.

> Or we might want to guarantee that we drop non-debug metadata, like:

Dropping non-debug metadata when stripping debug info sounds dangerous to me, since debug info is not supposed to have an impact on optimizations. (I was worried at first when I saw this patch because `heapallocsite` sounds like an optimization hint -- in which case I was going to suggest replacing the metadata with a remapped / equivalent version -- but `git grep` told me it's only used for debug info. Probably it should be renamed `dbg.heapallocsite` or something to make it clear that it shouldn't be used for anything else...)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98668