This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking][15/*] Account for assignment tracking in simplifycfg
ClosedPublic

Authored by Orlando on Sep 5 2022, 8:20 AM.

Details

Summary

A few of the changes in this patch update the behaviour from "delete debug intrinsics" to "move dbg.assigns to a new block and set them to undef". There's an argument that we could just keep the existing behaviour in these cases and accept that occasionally we will lose some debug info accuracy, for the sake of fewer special cases in llvm. Furthermore, we can't / don't always perform the "move + undef", such as in the case of DeleteDeadBlocks. There we do indeed just delete them for the sake of practicality. I don't particularly like this inconsistency, but it might just be the kind of thing we need to decide on a case by case basis.


I'd be keen to hear what others think about this one as I don't have a strong gut feeling for what is best, overall.

Diff Detail

Event Timeline

Orlando created this revision.Sep 5 2022, 8:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 8:20 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Orlando requested review of this revision.Sep 5 2022, 8:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 8:20 AM
jmorse added a subscriber: jmorse.Sep 9 2022, 4:28 AM

Soooo, for the TryToSimplifyUncondBranchFromEmptyBlock case I think this would expose the "LLVM can't represent things that happen on critical edges" issue [0], in that sinking the dbg.assigns out of a trivial block might put them in a block with other predecessors. IMO, it's preferable to maintain what dbg.values currently do, which is to vanish (sometimes causing wrong debug-info to be produced), because it's not a regression.

Addressing this is a much larger issue; possibly we can deal with that in the future, but I'd suggest not as part of this patch series. Same with the "hoist all..." situation -- that might be an improvement, but we should start off with no regressions, then consider the improvements we can make.

~

For the store-speculation that happens -- should there not be a call somewhere to merge the assign-IDs of these stores? (Possibly not). Plus, is there a possibility / chance that

  • stores are duplicated for some reason (unrolling?) and dbg.assigns with them, using different values,
  • one of them is speculated in SpeculativelyExecuteBB,
  • We end up setting the Value operand of all the dbg.assigns with the assign ID, to the speculated value, where some of them referred to a non-speculated value.

Maybe there's a reason why this doesn't happen. If it does, I suppose all that needs to happen is the assignment markers are filtered by whether their Value is the same as the speculated stores value.

(Note to self, didn't look at tests)

[0] https://github.com/llvm/llvm-project/issues/55115#issuecomment-1118420725

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2006–2007

This can (should!) be asserted; also, techncially can be peeled away from dbg.assign improvements if this also affects dbg.values?

jmorse requested changes to this revision.Sep 13 2022, 9:47 AM

(Sending back to "revise" queue given review above)

This revision now requires changes to proceed.Sep 13 2022, 9:47 AM
Orlando updated this revision to Diff 460381.Sep 15 2022, 5:48 AM
Orlando marked an inline comment as done.

Soooo, for the TryToSimplifyUncondBranchFromEmptyBlock case I think this would expose the "LLVM can't represent things that happen on critical edges" issue [0], in that sinking the dbg.assigns out of a trivial block might put them in a block with other predecessors. IMO, it's preferable to maintain what dbg.values currently do, which is to vanish (sometimes causing wrong debug-info to be produced), because it's not a regression.

Addressing this is a much larger issue; possibly we can deal with that in the future, but I'd suggest not as part of this patch series. Same with the "hoist all..." situation -- that might be an improvement, but we should start off with no regressions, then consider the improvements we can make.

In the past (prototype's past) this was a necessary change because dropping linked dbg.assign intrinsics led to incorrect locations. I think these changes have become less important now that we've introduced the "untagged stores still count as an assignment" rule. I have removed the changes - if this turns out to have caused trouble for some reason we can always re-evaluate.

For the store-speculation that happens -- should there not be a call somewhere to merge the assign-IDs of these stores? (Possibly not)

Good spot - I didn't realise I had missed the test for this in the patch. Looking at it again, it turns out my comment (in the code) is misleading, as the other block's store isn't actually deleted (/merged) with the speculated store (it is deleted by DSE later). I've updated the code comment and added speculated-store.ll.

Plus, is there a possibility / chance that
...

Yeah that sounds right. I've now guarded that with a check.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2006–2007

I spent some time trying to find an interesting source reproducer without assignment tracking but failed to found one. If you're happy with an artificial (/hand-written) test then I could move this into its own patch.

Just a couple of typos.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2008
3024

Thanks for taking a look

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

I was referring to the variable I there, but Instruction makes more sense and is not ambiguous. I'll update when I come to land this (or if major updates are required) if that's ok?

3024

I didn't add the comment above, but happy to fold in the update if you want.

Thanks for taking a look

My pleasure.

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

Yep!

3024

I see. I think it should be corrected but if you'd rather leave it alone, no worries.

jmorse accepted this revision.Nov 17 2022, 3:50 AM

LGTM, with one or two questions, and the middle test needs -simplifycfg adding

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

s/run/executed/, for formality?

llvm/test/DebugInfo/Generic/assignment-tracking/simplifycfg/sink-erase.ll
1 ↗(On Diff #460381)

This needs to run simplifycfg, right?

27–29 ↗(On Diff #460381)

Is that just checking a single block, and if not should there be an implicit-check-not happening?

This revision is now accepted and ready to land.Nov 17 2022, 3:50 AM
This revision was landed with ongoing or failed builds.Nov 18 2022, 2:17 AM
This revision was automatically updated to reflect the committed changes.
Orlando marked 5 inline comments as done.

Removed sink-erase.ll as it was testing the TryToSimplifyUncondBranchFromEmptyBlock changes that we removed from this patch earlier.

llvm/test/DebugInfo/Generic/assignment-tracking/simplifycfg/sink-erase.ll
27–29 ↗(On Diff #460381)

"Check that there is only one dbg.assign (for c) in the final block." -> "Check that the final block only contains one dbg.assign"

This test isn't actually needed anymore - it was testing the TryToSimplifyUncondBranchFromEmptyBlock changes that we removed from this patch earlier.