Page MenuHomePhabricator

[GVNHoist] fix a bug where GVNHoist preserves the debug location when it should be dropped
AcceptedPublic

Authored by yuanboli233 on Apr 9 2021, 12:40 AM.

Details

Reviewers
jmorse
dblaikie
Summary

It is a proposed patch for the bug presented here: https://bugs.llvm.org/show_bug.cgi?id=49099

In LLVM debug information update guide, "Hoisting identical instructions which appear in several successor blocks into a predecessor block. In this case, there is no single merged instruction. The rule for dropping locations applies."

Here in the code, the hoisted instruction keeps the previous debug location. If there are multiple of the instructions hoisted, then a conservative approach is possibly dropping the debug location.

I also provide a test case here.

If the patch makes sense, can you help commit it? Thanks.

Diff Detail

Event Timeline

yuanboli233 created this revision.Apr 9 2021, 12:40 AM
yuanboli233 requested review of this revision.Apr 9 2021, 12:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2021, 12:40 AM

updated with git-clang-format

You're preserving the location in the case that only one instruction is moved. Interestingly the linked document also says "a transformation should also preserve the debug location of an instruction that is moved between basic blocks, if the destination block already contains an instruction with an identical debug location." suggesting that you should only preserve the location if "an identical one" exists in the pred block. Scanning the block for an identical location sounds excessive. My gut reaction would be to simply add an additional check that the location of the instruction at the insertion point matches the (single) hoisted instruction's location before preserving the location, or alternatively always drop the location (i.e. remove the if guard altogether).

Digging deeper, it looks like this was discussed in the patch that introduces the wording to the document (D93662). cc @fhahn @vsk @jmorse. wdyt should happen here?

An aside: while looking at this I found a function Instruction::updateLocationAfterHoist() which looks like it might be useful, but all it does is call dropLocation().

Thanks for your feedback @Orlando! I checked the code again, and I agree with your opinion, it seems that removing the if guard will be a conservative solution for this case.

First updated to the version which simply drops the debug location, we may wait for other people's ideas.

aprantl added inline comments.
llvm/test/Transforms/GVNHoist/hoist_debugloc.ll
2

Could you add a comment explaining what this test is testing?

3

This test doesn't actually work for checking that there is no !dbg attachment on the instruction.
You probably want to add something like {{$}} at the end.

yuanboli233 updated this revision to Diff 336929.EditedApr 12 2021, 12:13 PM

I thought the previous CHECK will discard store instructions with debug location. Thanks for pointing it out @aprantl, I have added the comment and changed the CHECK. I have tested that the test case can now distinguish the versions before and after the proposed patch.

yuanboli233 marked 2 inline comments as done.Apr 13 2021, 6:47 PM

Orlando wrote:

the linked document also says "a transformation should also preserve the debug location of an instruction that is moved between basic blocks, if the destination block already contains an instruction with an identical debug location."

Digging deeper, it looks like this was discussed in the patch that introduces the wording to the document (D93662). cc @fhahn @vsk @jmorse. wdyt should happen here?

IIRC, that wording means "It's sufficient to leave the DebugLoc unaltered if it's placed in a block where that DebugLoc already appears". It isn't necessary to scan for identical DebugLocs, instead it avoids un-necessary dropping of DebugLocs when we already know it won't mislead the developer.

llvm/lib/Transforms/Scalar/GVNHoist.cpp
1143–1146

Wording change suggestion above; I think it's clearer to just say "if instructions are hoisted". As I understand the HowToUpdate... document, we drop source locations whenever we move instructions between blocks, no matter how many.

IMO, it's better to not refer to the debug-info-update-guide from source comments -- the comments should only describe the intention / purpose of the code. (This may be a style thing).

llvm/test/Transforms/GVNHoist/hoist_debugloc.ll
4

You can hard-code the "G" global variable name, LLVM is highly unlikely to change that as part of a future optimisation.

36

Un-necessary attributes should be removed.

Thank @jmorse for the suggestions, I have updated the diff to address them.

jmorse accepted this revision.Apr 23 2021, 8:14 AM

LGTM

This revision is now accepted and ready to land.Apr 23 2021, 8:14 AM

NB: do you need this and D100270 to be pushed for you?