[PATCH] D100845: [Debugify][Original DI] Test preservation of original debug var intrinsics in optimizations

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:465
+  bool Preserved = true;
+  for (const auto &V : DIFunctionsBefore) {
+    auto VarIt = DIFunctionsAfter.find(V.first);
----------------
djtodoro wrote:
> jmorse wrote:
> > Given that DIFunctionsBefore is a DenseMap, and the order of iteration here is being preserved by the `Bugs` array, won't this be vulnerable to DenseMaps non-deterministic iteration order? (Easily fixed by making everything `MapVector`s).
> We have used the `DenseMap` for each DI Metadata checking, since we thought the order of reported bugs doesn't matter; do you think we should care about it?
IMO yes: any kind of unexpected variation in the output is going to be fatal to anyone trying to script around this tooling. We should be conservative in what's outputted.


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:542
+
+    if (SP) {
       LLVM_DEBUG(dbgs() << "  Collecting subprogram: " << *SP << '\n');
----------------
djtodoro wrote:
> jmorse wrote:
> > The additions to this function seem very similar to the additions to collectDebugInfoMetadata, would it not benefit from a refactor / shared utilities? I don't know / feel enough about Debugify to know if there's some context I've missed.
> I guess there are existing lines as well that are very similar in both `collectDebugInfoMetadata()` and `checkDebugInfoMetadata()`. Can that refactoring be done as an incremental NFC patch?
Works for me.


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

https://reviews.llvm.org/D100845