Page MenuHomePhabricator

[DebugInfo][If-Converter] Update call site info during the optimization
ClosedPublic

Authored by NikolaPrica on Aug 29 2019, 9:08 AM.

Details

Summary

During the If-Converter optimization pay attention when copying or deleting call instructions in order to keep call site information in valid state.

Diff Detail

Event Timeline

NikolaPrica created this revision.Aug 29 2019, 9:08 AM
vsk added inline comments.Aug 29 2019, 11:23 AM
test/CodeGen/ARM/smml.ll
13 ↗(On Diff #217906)

How did you catch this?

Have you run the test suite using "./bin/llvm-lit -Dllc="llc -debug-entry-values"?

aprantl added inline comments.Aug 29 2019, 2:48 PM
lib/CodeGen/IfConversion.cpp
1779 ↗(On Diff #217906)

It looks like we will have to do something similar in a lot of places. Have you thought about a way to make this more automatic, for example by registering a hook on deleted instructions that gets called automatically (I don't know what kind of infrastructure we have, but I'm thinking of something akin to salvageDebugInfo())?

vsk added inline comments.Aug 29 2019, 2:51 PM
lib/CodeGen/IfConversion.cpp
1779 ↗(On Diff #217906)

I've asked about this before. The rationale for weeding out issues with an assert is to a) avoid slowing down each MachineInstr::eraseFromParent call and b) allow for the possibility of preserving call site information on inst deletion rather than throwing it away. This makes sense to me: I'm satisfied that we have an assert that lets us find the places call site info needs to be updated.

aprantl added inline comments.Aug 29 2019, 4:18 PM
lib/CodeGen/IfConversion.cpp
1779 ↗(On Diff #217906)

I was unaware (or forgot about) the assert. That's fine with me then!

NikolaPrica marked an inline comment as done.Aug 30 2019, 2:16 AM
NikolaPrica added inline comments.
test/CodeGen/ARM/smml.ll
13 ↗(On Diff #217906)

I've built couple of binaries that triggered the assertion and then resolved it. Since I was testing for the ARM I just replaced updatedCallSiteInfo with an assertion and run all ARM tests to see whether it will trigger something. But the call you mentioned is simpler one! I will try it in future! Thanks!

vsk accepted this revision.Aug 30 2019, 9:20 AM

FWIW, this looks good to me, but it would be great to get another +1 as I'm not familiar with this pass.

This revision is now accepted and ready to land.Aug 30 2019, 9:20 AM

Should we look for another reviewer or this is good to go?

efriedma added inline comments.
lib/CodeGen/IfConversion.cpp
2065 ↗(On Diff #217906)

This comment doesn't make sense; what's getting deleted?

Thanks for the comment @efriedma! I've looked more carefully this optimization. It turns out that after CopyAndPredicateBlock it is not guaranteed that copied instruction block will be deleted. The deletion of critical call instruction happens after If-conversion during the BranchFolding cleaning when some block does not have predecessors.

Since we are not sure whether the block will be deleted after CopyAndPredicateBlock I've extended updateCallSiteInfo to be able to just make a copy of call site info.

efriedma added inline comments.Sep 13 2019, 11:53 AM
include/llvm/CodeGen/MachineFunction.h
987 ↗(On Diff #220107)

Can we split this API into multiple entry points? It now has three different modes with different semantics, which is more confusing than helpful.

Split call site updating functions on move, copy and erase functions.

vsk added a comment.Tue, Sep 17, 11:27 AM

Splitting up the updateCallSiteInfo API is a great idea. Some minor comments inline.

include/llvm/CodeGen/MachineFunction.h
996 ↗(On Diff #220477)

nit: s/the//

1001 ↗(On Diff #220477)

nit: s/we are removing/to remove/

Adjust comments in MachineFunction.h

efriedma accepted this revision.Thu, Sep 19, 3:53 PM

LGTM

lib/CodeGen/BranchFolding.cpp
168 ↗(On Diff #220641)

This looks fine, but did you mean to include it here?

NikolaPrica marked an inline comment as done.Thu, Sep 19, 11:57 PM
NikolaPrica added inline comments.
lib/CodeGen/BranchFolding.cpp
168 ↗(On Diff #220641)

Yes. I meant to include it here. The assertion from MachineFunction::DeleteMachineInstr was triggered here. With analysis I came to the conclusion that it is a consequence of removing predecessors of MachineBasicBlock after calling IfConverter::CopyAndPredicateBlock in IfConvertSimple or IfConvertTriangle optimizations. At some point certain MBB will not have any predecessors and thus it will be removed during the BranchFolding optimization which also removes dead blocks. Without this part of code test case "DebugInfo/MIR/ARM/if-coverter-call-site-info.mir" would trigger mentioned assertion.

efriedma added inline comments.Tue, Sep 24, 1:04 PM
lib/CodeGen/BranchFolding.cpp
168 ↗(On Diff #220641)

Ok, that makes sense.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Oct 8, 8:45 AM
Herald added a subscriber: hiraditya. · View Herald Transcript