[PATCH] D40512: WIP: [Debugify] Add a pass to test debug info preservation

[Davide Italiano via Phabricator via llvm-commits]

davide requested changes to this revision.
davide added a comment.
This revision now requires changes to proceed.

Thank you Vedant, some comments inline.



================
Comment at: lib/Transforms/Utils/Debugify.cpp:60-62
+    auto CU = DIB.createCompileUnit(dwarf::DW_LANG_C_plus_plus_11,
+                                    DIB.createFile(M.getName(), "/"),
+                                    "debugify", /*isOptimized=*/true, "", 0);
----------------
ehm, are you sure this is right?
This will break for code emitted by another frontend.


================
Comment at: lib/Transforms/Utils/Debugify.cpp:77-81
+        for (Instruction &I : BB)
+          I.setDebugLoc(DILocation::get(Ctx, NextLine++, 1, SP));
+
+        // Attach debug values.
+        for (Instruction &I : BB) {
----------------
is there a way to avoid two scans of the whole module?


================
Comment at: lib/Transforms/Utils/Debugify.cpp:148
+    // Find missing lines.
+    BitVector MissingLines{OriginalNumLines, true};
+    for (Function &F : M) {
----------------
Why a bitvector?


================
Comment at: lib/Transforms/Utils/Debugify.cpp:149-151
+    for (Function &F : M) {
+      for (BasicBlock &BB : F) {
+        for (Instruction &I : BB) {
----------------
You can probably use
`F.instructions()` here.


================
Comment at: lib/Transforms/Utils/Debugify.cpp:173-175
+    for (Function &F : M) {
+      for (BasicBlock &BB : F) {
+        for (Instruction &I : BB) {
----------------
I think you can simplify this writing

```
for (Instruction &I: F.instructions())
```

or something [check `include/llvm/IR/`]


================
Comment at: lib/Transforms/Utils/Debugify.cpp:181-183
+          bool Valid = to_integer(DVI->getVariable()->getName(), VarNum, 10);
+          assert(Valid && "Unexpected name for DILocalVariable");
+          (void)Valid;
----------------
Can you fold valid inside the assertion and avoid the 
`(void)Valid` hack?


================
Comment at: lib/Transforms/Utils/Debugify.cpp:212
+
+INITIALIZE_PASS_BEGIN(Debugify, "debugify", "Attach debug info to everything",
+                      false, false)
----------------
davide wrote:
> `INITIALIZE_PASS` will suffice.
I think the 3rd or 4th parameter should be true? 
If this preserves everything, also preserves the CFG?

Also, isn't something that preserves everything (i.e. doesn't mutate the IR) technically an analysis?


================
Comment at: lib/Transforms/Utils/Debugify.cpp:212-215
+INITIALIZE_PASS_BEGIN(Debugify, "debugify", "Attach debug info to everything",
+                      false, false)
+INITIALIZE_PASS_END(Debugify, "debugify", "Attach debug info to everything",
+                    false, false)
----------------
`INITIALIZE_PASS` will suffice.


https://reviews.llvm.org/D40512