[PATCH] D82545: [Debugify] Make the debugify aware of the original (-g) Debug Info

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

@djtodoro sorry for the delay here. This looks much nicer now.



================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:329
    # Suppresses verbose debugify output.
-   $ opt -enable-debugify -debugify-quiet -pass-to-test sample.ll
+   $ opt -enable-debugify=synthetic/original -debugify-quiet -pass-to-test sample.ll
 
----------------
If 'synthetic/original' is not literally a string that can be passed to the -enable-debugify cl::opt, I think this example will be misleading. Perhaps we can sidestep the issue by picking a default behavior for -enable-debugify?


================
Comment at: llvm/include/llvm/Transforms/Utils/Debugify.h:31
+struct DebugInfoPerPass {
+  // This maps a function name and its DISubprogram.
+  DebugFnMap DIFunctions;
----------------
s/and its/to its associated/?


================
Comment at: llvm/include/llvm/Transforms/Utils/Debugify.h:33
+  DebugFnMap DIFunctions;
+  // This maps an instruction and the info whether it has !dbg attached.
+  DebugInstMap DILocations;
----------------
nit: 'about whether'?


================
Comment at: llvm/include/llvm/Transforms/Utils/Debugify.h:156
+  bool EnableDebugifyEachSynthetic = false;
+  bool EnableDebugifyEachOriginal = false;
 
----------------
This can be simplified by just storing a single DebugifyMode enum.


================
Comment at: llvm/include/llvm/Transforms/Utils/Debugify.h:194
+            &DIPreservationMap));
+      }
       break;
----------------
We shouldn't need two sets of super::add calls here. We should simply pass in the DebugifyMode enum, as well as the rest of the optional arguments that might be needed in original mode.


================
Comment at: llvm/include/llvm/Transforms/Utils/Debugify.h:208
+            false, Name, nullptr, DebugifyMode::OriginalDebugInfo,
+            &DIPreservationMap));
+      }
----------------
Ditto, I don't think we need two sets of super::add calls here either.


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:310
+// This checks the preservation of original debug info attached to functions.
+static bool checkFunctions(DebugFnMap &DIFunctionsBefore,
+                           DebugFnMap &DIFunctionsAfter,
----------------
Does this need to be `const DebugFnMap &`?


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:316
+  for (const auto &F : DIFunctionsAfter) {
+    if (!F.second) {
+      auto SPIt = DIFunctionsBefore.find(F.first);
----------------
Early continue?


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:322
+              << FileNameFromCU << '\n';
+        if (Preserved)
+          Preserved = false;
----------------
No need to check `if (Preserved)` before assigning it -- we can leave that sort of optimization to the compiler.


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:323
+        if (Preserved)
+          Preserved = false;
+      } else {
----------------
Early continue here as well?


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:331
+                << F.first << " from " << FileNameFromCU << '\n';
+          if (Preserved)
+            Preserved = false;
----------------
Ditto.


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:349
+  for (const auto &L : DILocsAfter) {
+    if (!L.second) {
+      auto Instr = L.first;
----------------
Early continue?


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:355
+
+      auto InstrIt = DILocsBefore.find(Instr);
+      if (InstrIt == DILocsBefore.end()) {
----------------
Is `Instr` an `llvm::Instruction *`? I wonder whether it is sound to assume that pointer equality for `llvm::Instruction` implies actual equality, if we're comparing pointers before/after a pass runs. It's possible for a pass to delete and then create instructions: that could result in pointer reuse/recycling.


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:357
+      if (InstrIt == DILocsBefore.end()) {
+        dbg() << "ERROR: " << NameOfWrappedPass
+              << " did not generate DILocation for " << *Instr
----------------
This might need to be relaxed to a warning, since we do specifically recommend that pass authors drop debug locations in certain transformations.


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:411
+          {F.getName(), nullptr});
+    }
+
----------------
There can be a single map insertion call here, the branch is only needed to pretty-print SP when it's non-null.


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

https://reviews.llvm.org/D82545