We may have a better solution. But it make non sense to insert the EndCf in the unreachable block when the unreachable instruction is the first instruction in the block.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
I don't agree that it doesn't make sense to put the end.cf into an unreachable block. If it ordinarily would go into a block in that place, it makes sense. This needs IR checklines for the intrinsic insertion points
This needs tome tests where there are instructions before the unreachable. If other instruction like an abort were there, the end cf still needs to be inserted
I think the key is the FirstInsertionPt in the BB is a Unreachable. This should essentially guarantee no other instructions before this Unreachable (PHI, Landing Pad?).
I think we are safe here not to insert the end cf. And we may need come informative comments here.
Update the test.
This patch fixes the "Instruction does not dominate all uses!" when we try to insert EndCF in an unreachable block where unreachable is
the first instruction in the block. Actually we don't insert ENDCF in such case.
However if there are instructions before unreachable in the block, we should insert the EndCF intrinsics, and we will hit
the "Instruction does not dominate all uses!" issue.
test/CodeGen/AMDGPU/si-annotate-cf-unreachable.ll | ||
---|---|---|
30–34 ↗ | (On Diff #64790) | There should still be another copy of this function with blocks with code (like a volatile store) before the unreachable |
test/CodeGen/AMDGPU/si-annotate-cf-unreachable.ll | ||
---|---|---|
30–34 ↗ | (On Diff #64790) | The issue is: for an unreachable block, if we insert EndCf, we will hit "Instruction does not dominate all uses!" assert! The "fix" here is actually just a workaround: if there is no instruction before unreachable, we don't insert EndCf. If as you suggested there are instructions before unreachable, we will have to insert endCF, and the problem is still there. This is a special case in control flow that both "else" and "then" is unreachable. Maybe we need to insert endCF on |
test/CodeGen/AMDGPU/si-annotate-cf-unreachable.ll | ||
---|---|---|
30–34 ↗ | (On Diff #64790) | You check at the insertion point if it is unreachable, not that the block ends in unreachable. That's what I want the other test for, to make sure it doesn't crash in that case. I think what you might really want to be doing is checking for no successors |
test/CodeGen/AMDGPU/si-annotate-cf-unreachable.ll | ||
---|---|---|
30–34 ↗ | (On Diff #64790) | Can you use a simple example to show me what do you want to test? I am afraid that the test you request should still fail with this patch. |
test/CodeGen/AMDGPU/si-annotate-cf-unreachable.ll | ||
---|---|---|
30–34 ↗ | (On Diff #64790) | bb4: store volatile i32 0, i32 addrspace(1)* undef unreachable This patch should just be an optimization? It is not correct to do something for correctness assuming there is only one instruction in the block ending in unreachable |
LGTM, I need this for other related fixes. This doesn't really solve the underlying issue here though