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

[Vedant Kumar via Phabricator via llvm-commits]

vsk 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);
----------------
davide wrote:
> 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.
I don't think there's any correct language value we could use here due to the issue you point out. In practice, it shouldn't matter unless you actually try to load a debugified module in a debugger, which is a non-goal. I can keep things simple here by just using DW_LANG_C.


================
Comment at: lib/Transforms/Utils/Debugify.cpp:148
+    // Find missing lines.
+    BitVector MissingLines{OriginalNumLines, true};
+    for (Function &F : M) {
----------------
davide wrote:
> 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).
Gotcha. I didn't benchmark against DenseMap -- I just visualized 'MissingLines' as a bitmap and went down that route.


================
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:
> 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")` ?
Then we'd need to call to_integer() twice (for the non-asserts and asserts paths), which doesn't seem better.


================
Comment at: lib/Transforms/Utils/Debugify.cpp:212
+
+INITIALIZE_PASS_BEGIN(Debugify, "debugify", "Attach debug info to everything",
+                      false, false)
----------------
davide wrote:
> 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.
Based on docs/Passes, I think CheckDebugify best fits under the category of Utility passes. It doesn't provide information for other passes and it doesn't transform the IR.

The Debugify pass does technically provide information for another pass, but IMO it doesn't make sense to try to preserve/cache/invalidate its results, so it doesn't quite seem like an analysis either.


https://reviews.llvm.org/D40512