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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Minor inline nits that can be skipped, plus a question about relying on DenseMap ordering.

Looks good, although I've no familiarity with the python.



================
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) {
----------------
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).


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


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


================
Comment at: llvm/unittests/Transforms/Utils/DebugifyTest.cpp:66-68
+    if (Dbgs.empty())
+      return false;
+
----------------
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).


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

https://reviews.llvm.org/D100845