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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 4 inline comments as done.
djtodoro added a comment.

@aprantl @vsk Thanks a lot for the reviews!



================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:221
 
 The ``debugify`` utility
 ^^^^^^^^^^^^^^^^^^^^^^^^
----------------
aprantl wrote:
> Can we call this
> ```
> The ``debugify`` utility pass
> ```
> ?
> 
> I kind of expect a utility to be a standalone executable.
I agree with that.


================
Comment at: llvm/docs/HowToUpdateDebugInfo.rst:230
+
+* `original` - collects and checks the preservation of real debug info
+  metadata
----------------
aprantl wrote:
> I think that this is a useful feature, but from the user's perspective the naming is confusing.
> 
> The name `debugify` implies that that the tool is taking input and then adds something debug-related to it. That clashes with preserving original information.
> 
> My suggestion is to either rename debugify to something more generic that encompasses both use-cases, or to share the implementation in one class, but give it two separate user-facing names. Maybe separating out the debugify action from the check action would also work (`--enable-debugify --enable-debuginfo-(preservation-)verification`).
Sure, I'll try to find a name for the pass(es) that will cover both use-cases. Thanks!


================
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
 
----------------
vsk wrote:
> 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?
I'll try to make some changes to avoid ambiguity here.


================
Comment at: llvm/include/llvm/Transforms/Utils/Debugify.h:27
+using DebugFnMap = std::map<llvm::StringRef, const llvm::DISubprogram *>;
+using DebugInstMap = std::map<const llvm::Instruction *, bool>;
+
----------------
aprantl wrote:
> Why is this a std::map and not a DenseMap? Do we need the map's sorted-ness?
Oh, I've forgotten why I have (initially) used the `std::map` here, but yes, we don't need the balanced data structure for this, indeed. Thanks!


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


================
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;
----------------
vsk wrote:
> nit: 'about whether'?
I agree, thanks.


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


================
Comment at: llvm/include/llvm/Transforms/Utils/Debugify.h:194
+            &DIPreservationMap));
+      }
       break;
----------------
vsk wrote:
> 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.
Good point. I was planning to do the final refactoring, but I haven't had time for that, thanks for catching this!


================
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,
----------------
vsk wrote:
> Does this need to be `const DebugFnMap &`?
Yes, I'll add it to some other spots as well.


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


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


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:355
+
+      auto InstrIt = DILocsBefore.find(Instr);
+      if (InstrIt == DILocsBefore.end()) {
----------------
vsk wrote:
> 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.
It is an `llvm::Instruction *`. I see... any idea how to address that?
(I was trying to avoid having to make a temp object of the instr...)


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:357
+      if (InstrIt == DILocsBefore.end()) {
+        dbg() << "ERROR: " << NameOfWrappedPass
+              << " did not generate DILocation for " << *Instr
----------------
vsk wrote:
> This might need to be relaxed to a warning, since we do specifically recommend that pass authors drop debug locations in certain transformations.
Agree.. We can extend this (one day) to catch the cases where we actually allow dropping of the di locations, but until then, it is ok to be a warning.


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

https://reviews.llvm.org/D82545