This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking][11/*] Update RemoveRedundantDbgInstrs
ClosedPublic

Authored by Orlando on Sep 5 2022, 5:14 AM.

Details

Summary

Update the RemoveRedundantDbgInstrs utility to avoid sometimes losing information when deleting dbg.assign intrinsics.

removeRedundantDbgInstrsUsingBackwardScan - treat dbg.assign intrinsics that are not linked to any instruction just like dbg.values. That is, in a block of contiguous debug intrinsics, delete all other than the last definition for a fragment. Leave linked dbg.assign intrinsics in place.

removeRedundantDbgInstrsUsingForwardScan - this function has two changes. First, undef and unlinked dbg.assign intrinsics encountered in the entry block before any non-undef non-unlinked intrinsics for a given fragment are deleted. Second, don't delete linked dbg.assign intrinsics and don't the next intrinsic found even if it would otherwise be eligible for deletion.

Diff Detail

Event Timeline

Orlando created this revision.Sep 5 2022, 5:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 5:14 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Orlando requested review of this revision.Sep 5 2022, 5:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 5:14 AM
jmorse added a subscriber: jmorse.Sep 7 2022, 5:04 AM

If I understand the logic in the forward scan correctly, whenever you see a linked dbg.assign, you will _always_ keep the next debug intrinsic for that variable, because the expression is nullptr. The next debug intrinsic then resets the map, so if it's a dbg-value-like intrinsic, subsequent dbg-value-like intrinsics will be removed as redundant.

I can sort of see why this is necessary (moving a variable from "linked" to "not linked" is a meaningful transition that we shouldn't remove), is there a definite use case for it, and is it covered by the test? (I don't see it being covered by the test, but I might not have read it correctly).

As annoying as this is: the codepaths using SeenDefForAggregate should be tested by the test too, it'll make it easier for me to grasp what's going on.

llvm/test/DebugInfo/Generic/assignment-tracking/remove-redundant.ll
18–19

... in the entry block, being the important feature, no?

51–55

Deliberately identical to lines 44 -> 48?

If I understand the logic in the forward scan correctly, whenever you see a linked dbg.assign, you will _always_ keep the next debug intrinsic for that variable, because the expression is nullptr. The next debug intrinsic then resets the map, so if it's a dbg-value-like intrinsic, subsequent dbg-value-like intrinsics will be removed as redundant.

I can sort of see why this is necessary (moving a variable from "linked" to "not linked" is a meaningful transition that we shouldn't remove), is there a definite use case for it, and is it covered by the test? (I don't see it being covered by the test, but I might not have read it correctly).

Yeah that's right. I thought I had covered it in the tests but looking again I think you're right.

is there a definite use case for it?

This is a good question - I think so, but I tried to come up with a source reproducer and failed to do so quickly. I'll need to think about this one some more.

llvm/test/DebugInfo/Generic/assignment-tracking/remove-redundant.ll
18–19

Yes for sure, I'll add that to the comment.

51–55

Yeah - the comments don't make it particularly clear or obvious but the difference is that the shadowing intrinsic is a dbg.value in the first check and a dbg.assign in the second.

;; redundant by the previous dbg.value is removed.

;; redundant by the previous dbg.assign is removed.

I can improve the wording there to make it clearer.

57–60

This demonstrates the SeenDefForAggregate path - we're in the entry block and this is a unlinked and undef dbg.assign (so far it's a candidate for removal). We don't want to remove it though because we've already seen non-undef or non-unlinked intrinsic for the same variable.

Essentially, scanning forwards, we can only remove an undef unlinked dbg.assign from entry block iff we've not already seen a def for the variable.

Orlando planned changes to this revision.Sep 13 2022, 10:06 AM
Orlando updated this revision to Diff 460004.Sep 14 2022, 1:20 AM
Orlando marked 2 inline comments as done.

I can sort of see why this is necessary (moving a variable from "linked" to "not linked" is a meaningful transition that we shouldn't remove), is there a definite use case for it, and is it covered by the test? (I don't see it being covered by the test, but I might not have read it correctly).

I've added remove-redundant-fwd-scan-linked.ll which is a test case reduced from real code. Note, this IR was generated using the (updated) DSE insert-undef behaviour in D133315.

Orlando updated this revision to Diff 460006.Sep 14 2022, 1:23 AM

+ Improve the new test by checking DIAssignID instances are unique.

(Sorry for the double-update)

jmorse accepted this revision.Sep 14 2022, 4:02 AM

LGTM. Due diligence: is there a scenario you can think of where there would be meaningful difference in the generated debug-info if the tested-for dbg.assign in remove-redundant-fwd-scan-linked.ll were dropped? As mentioned it feels like a meaningful transition (linked -> unlinked), but I can't put my finger on exactly what it means.

llvm/test/DebugInfo/Generic/assignment-tracking/remove-redundant.ll
18
This revision is now accepted and ready to land.Sep 14 2022, 4:02 AM
Orlando updated this revision to Diff 474251.Nov 9 2022, 6:18 AM
Orlando added a subscriber: StephenTozer.

+ Separate out the entry-block-undef-removal into it's own scan, in line with @StephenTozer 's comments in D136321.
+ Rebase.
+ Switch to opaque pointers in tests.

Orlando requested review of this revision.Nov 9 2022, 6:19 AM
Orlando added a reviewer: StephenTozer.
jmorse accepted this revision.Nov 16 2022, 4:01 AM

LGTM with some cosmetic nits.

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
439

Probably wants a comment to summarise the purpose, i.e., "dbg.assigns with no linked instructions can be...". I know there's a similar comment below, but IMO it makes more sense to put that comment up here.

525

Early-continue better than indentation IMO.

534

Best to be 100% explicit -- push DAI, and change the type of the container to be DbgAssignInst?

540

Nit, auto * rather than a reference to the pointer? I vaguely recall some past compilers issuing diagnostics about this.

This revision is now accepted and ready to land.Nov 16 2022, 4:01 AM
This revision was landed with ongoing or failed builds.Nov 16 2022, 4:28 AM
This revision was automatically updated to reflect the committed changes.
Orlando marked 4 inline comments as done.

Done - thanks!

StephenTozer added inline comments.Nov 16 2022, 4:50 AM
llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
392–393
Orlando added inline comments.Nov 16 2022, 5:11 AM
llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
392–393

I think the continue is correct (this isn't the end of the loop, it's inside the if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(&I)) block) - continue because we're still in a block of consecutive dbg intrinsics.

StephenTozer added inline comments.Nov 17 2022, 4:47 AM
llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
392–393

Ah, my bad - just misread it! LGTM.