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

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
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);
----------------
vsk wrote:
> 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?
The scenario I have in mind is the one where you have rust emitting an IR module  without debug info and then running this pass on it (where you get a C++11 compile unit).
I guess there's no real to understand which frontend is emitting the IR unless the frontend emits enough metadata for the mid-level optimizer to find out which language is this IR derived from.

Another example is, what if you have code compiled with `-std=c++98/-std=c++03`, without debuginfo on which you call this pass?
I think in that case is setting  `C_plus_plus_11` always correct? I'm not sure myself, so please correct me if I'm wrong.


================
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) {
----------------
vsk wrote:
> 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.
Fair enough. I'm just slightly worried about LTO as scanning the whole module can become really expensive. I guess we'll measure and see :)


================
Comment at: lib/Transforms/Utils/Debugify.cpp:148
+    // Find missing lines.
+    BitVector MissingLines{OriginalNumLines, true};
+    for (Function &F : M) {
----------------
vsk wrote:
> 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.
Sounds reasonable. I thought you benchmarked vs DenseMap and found it a big win.
FWIW, if you have lots of setrange operations, it definitely is, in fact some algorithms use it as a cheap worklist (e.g. NewGVN).


================
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;
----------------
vsk wrote:
> 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.
even if you write
`assert(to_integer() && "patatino")` ?


================
Comment at: lib/Transforms/Utils/Debugify.cpp:212
+
+INITIALIZE_PASS_BEGIN(Debugify, "debugify", "Attach debug info to everything",
+                      false, false)
----------------
vsk wrote:
> 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.
Could be (at least in my opinion). I'm not sure whether metadata added are considered something IR transformations, as passes can just ignore them willy-nilly.
I think Chandler is the best person to comment on this one after years on the pass manager design. Ideally, you might also bring this up on llvm-dev.


https://reviews.llvm.org/D40512