During the If-Converter optimization pay attention when copying or deleting call instructions in order to keep call site information in valid state.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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"? |
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())? |
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. |
lib/CodeGen/IfConversion.cpp | ||
---|---|---|
1779 ↗ | (On Diff #217906) | I was unaware (or forgot about) the assert. That's fine with me then! |
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! |
FWIW, this looks good to me, but it would be great to get another +1 as I'm not familiar with this pass.
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.
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. |
LGTM
lib/CodeGen/BranchFolding.cpp | ||
---|---|---|
168 ↗ | (On Diff #220641) | This looks fine, but did you mean to include it here? |
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. |
lib/CodeGen/BranchFolding.cpp | ||
---|---|---|
168 ↗ | (On Diff #220641) | Ok, that makes sense. |