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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:305-306
     // Collect the DISubprogram.
     auto *SP = F.getSubprogram();
     DIPreservationMap[NameOfWrappedPass].DIFunctions.insert({F.getName(), SP});
+    if (SP) {
----------------
jmorse wrote:
> I know this isn't code changed by this patch; but can't we early-exit if there's no subprogram here? As far as I understand it, no subprogram means that there are no debugging intrinsics or !dbg attachments to be found in the function (and anything otherwise is a verifier error).
There could be a case where we have forgotten to do `F->setSubprogram(SP)` when doing some optimizations on `F` (more precisely, `newF->setSubprogram(SP)` -- but `newF` is representing the same function `F`). I've found a case like that when doing full LLVM-projects build.


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:465
+  bool Preserved = true;
+  for (const auto &V : DIFunctionsBefore) {
+    auto VarIt = DIFunctionsAfter.find(V.first);
----------------
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?


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:542
+
+    if (SP) {
       LLVM_DEBUG(dbgs() << "  Collecting subprogram: " << *SP << '\n');
----------------
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?


================
Comment at: llvm/unittests/Transforms/Utils/DebugifyTest.cpp:66-68
+    if (Dbgs.empty())
+      return false;
+
----------------
jmorse wrote:
> Mega nit, this is redundant given the loop below simply won't iterate over Dbgs if it's empty, no? (This might be a style thing).
It makes sense :)


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

https://reviews.llvm.org/D100845