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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

Thanks for your comments Davide!



================
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);
----------------
davide wrote:
> ehm, are you sure this is right?
> This will break for code emitted by another frontend.
I really want to say "pick any language" here. This pass ignores IR which already has debug info, so I'm not sure what breaks -- could you clarify?


================
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) {
----------------
davide wrote:
> is there a way to avoid two scans of the whole module?
These loops could be combined. IMO it's just slightly neater to write the early break in the second loop if it's a separate loop.


================
Comment at: lib/Transforms/Utils/Debugify.cpp:148
+    // Find missing lines.
+    BitVector MissingLines{OriginalNumLines, true};
+    for (Function &F : M) {
----------------
davide wrote:
> Why a bitvector?
It seems like a cheap way to track whether or not we've seen each line. I originally wanted to use an IntervalMap, but it has no mechanism to remove a mapping, or to iterate over holes in the mapping.


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


================
Comment at: lib/Transforms/Utils/Debugify.cpp:173-175
+    for (Function &F : M) {
+      for (BasicBlock &BB : F) {
+        for (Instruction &I : BB) {
----------------
davide wrote:
> I think you can simplify this writing
> 
> ```
> for (Instruction &I: F.instructions())
> ```
> 
> or something [check `include/llvm/IR/`]
Will fix.


================
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;
----------------
davide wrote:
> Can you fold valid inside the assertion and avoid the 
> `(void)Valid` hack?
AFAIK we need the hack to avoid -Wunused in non-asserts builds.


================
Comment at: lib/Transforms/Utils/Debugify.cpp:212
+
+INITIALIZE_PASS_BEGIN(Debugify, "debugify", "Attach debug info to everything",
+                      false, false)
----------------
davide wrote:
> 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?
Thanks for the catch! Do you think this belongs in lib/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)
----------------
vsk wrote:
> davide wrote:
> > 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?
> Thanks for the catch! Do you think this belongs in lib/Analysis?
Will fix.


https://reviews.llvm.org/D40512