Page MenuHomePhabricator

Recover debug intrinsics when killing duplicate or empty basic blocks
ClosedPublic

Authored by StephenTozer on Fri, Nov 15, 8:55 AM.

Details

Summary

Fixes the bug: https://bugs.llvm.org/show_bug.cgi?id=39339

When basic blocks are killed, either due to being empty or to being an if.then or if.else block whose complement contains identical instructions, the debug intrinsics in that block are lost. This fix attempts to recover those intrinsics by either hoisting or sinking them if possible, or replacing them in the above block with Undef values to prevent debug info from falling out-of-date.

Diff Detail

Event Timeline

StephenTozer created this revision.Fri, Nov 15, 8:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 15, 8:55 AM
vsk added a subscriber: efriedma.Fri, Nov 15, 9:51 AM
vsk added inline comments.
llvm/lib/Transforms/Utils/Local.cpp
1050

I had to squint at this for a while to figure out why it's sufficient. Could you clear this up by writing the loop as for (auto I : *BB), then asserting isa<DbgInfoIntrinsic>(&I) || I.isTerminator()?

1056

Could you add a new helper to Local.h to share this wrap-in-undef logic with llvm::replaceDbgUsesWithUndef (above)?

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1287

Is any special/extra work needed to handle duplicate dbg.values for a DebugVariable that's already present in the UndefDVIs map? The first one that's inserted into the map is hoisted and converted to an 'undef' -- what happens to the rest?

1402

You can use the set-to-undef helper here.

This seems fine, generally.

Would it be possible to salvage the debug info using the branch condition? Maybe complicated, but it seems like it should be possible, at least in theory. (Probably better to leave that for a followup, though.)

rnk added a subscriber: rnk.Sun, Nov 17, 9:06 AM
rnk added inline comments.
llvm/include/llvm/IR/DebugInfoMetadata.h
3253 ↗(On Diff #229567)

This is a good idea whose time has come. :) I think in the long run, we should make this concept more explicit and first-class in our IR representation, rather than something we work out by looking at fragments.

jmorse added a subscriber: jmorse.Tue, Nov 19, 3:44 AM
jmorse added inline comments.
llvm/lib/CodeGen/LiveDebugValues.cpp
169 ↗(On Diff #229567)

NB, the current patch doesn't apply to trunk/master. This line (169) and the constructor on 195 were removed in r373727

StephenTozer updated this revision to Diff 230090.EditedTue, Nov 19, 9:07 AM

Updating according to review comments; the code in Local.cpp needs further work, but is close to its likely final state. This version also applies to master, so diffing the latest revision with the previous will give a lot of noise.

aprantl added inline comments.Tue, Nov 19, 10:10 AM
llvm/include/llvm/IR/DebugInfoMetadata.h
3257 ↗(On Diff #230090)

That comment is a bit vague for DebugInfoMetadata.

Does this represent exactly one non-overlapping fragment of an inlined instance of a variable? We should say so. Since this isn't used anywhere in this file, there should be an example for when to use this.

3258 ↗(On Diff #230090)

Can you break this out into a separate NFC commit with a unit test?

aprantl added a subscriber: Orlando.
aprantl added inline comments.
llvm/include/llvm/IR/DebugInfoMetadata.h
3258 ↗(On Diff #230090)
StephenTozer marked an inline comment as done.Wed, Nov 20, 1:38 AM
StephenTozer added inline comments.
llvm/include/llvm/IR/DebugInfoMetadata.h
3258 ↗(On Diff #230090)

What sort of test coverage would be appropriate for a class like this?

NB, you'll also want to remove the DebugVariable portions of this patch and assign it as a child revision of D70486, and the tests went missing in the latest revision :o)

Adrian wrote:

aprantl added a reviewer: jmorse.

Full disclosure, Stephen is also a Sony-ite, and I believe organisation-internal approves are generally frowned on. I'm happy with the general direction and implementation of this patch though.

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp
805–807 ↗(On Diff #230090)

Something that accidentally snuck in?

llvm/lib/Transforms/Utils/Local.cpp
1051

While building with this patch elsewhere I ran into some assertions, from isel observing dbg.declare's with Undef operands. This is actually great: it means that beforehand we weren't just losing dbg.values, but dbg.declares describing un-SROA'd/un-mem2reg'd locations too.

Those dbg.declares need to have their operands preserved rather than set to undef -- they're not control flow dependent, so it doesn't matters where they get placed (I think, 90%).

Show correct changeset, preserve operands for dbg.declares

Please make the stack of patches (the D70486 should be parent for this one).

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1399

clang-format please

I think the patches in the stack should be in the opposite direction (firstly D70486 and then D70486).

Fix formatting, include whole files in patch (mistakenly removed in prior revision).

Adrian wrote:

aprantl added a reviewer: jmorse.

Full disclosure, Stephen is also a Sony-ite, and I believe organisation-internal approves are generally frowned on. I'm happy with the general direction and implementation of this patch though.

@jmorse you demonstrably have experience in this area, so it's entirely appropriate for you to participate in the upstream reviews. Just don't let it go to your head.

aprantl added inline comments.Fri, Nov 22, 8:42 AM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1268

Does this get easier to read if you factor out the two while loops into a sub-lambda and give that a descriptive name?

Use another lambda to simplify skipping of dbg info, fix incorrect type usage in setDbgVariableUndef

aprantl accepted this revision.Mon, Dec 2, 1:39 PM
This revision is now accepted and ready to land.Mon, Dec 2, 1:39 PM

Maintain order of debug intrinsics in empty block elimination, fix test

Use Undef int1 when undef-ing a DbgVariable without a type.

This revision was automatically updated to reflect the committed changes.