This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/SI: Do not insert EndCf in an unreachable block
ClosedPublic

Authored by cfang on Jul 5 2016, 4:45 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

cfang updated this revision to Diff 62803.Jul 5 2016, 4:45 PM
cfang retitled this revision from to AMDGPU/SI: Do not insert EndCf in an unreachable block.
cfang updated this object.
cfang added reviewers: arsenm, tstellarAMD.
cfang added subscribers: arsenm, llvm-commits.
arsenm edited edge metadata.Jul 5 2016, 4:55 PM

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

arsenm added a comment.Jul 8 2016, 8:57 AM

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

cfang added a comment.Jul 8 2016, 12:39 PM

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.

cfang updated this revision to Diff 64790.Jul 20 2016, 4:16 PM
cfang updated this object.
cfang edited edge metadata.

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.

arsenm added inline comments.Jul 22 2016, 10:22 AM
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

cfang added inline comments.Jul 22 2016, 10:37 AM
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
both paths.

arsenm added inline comments.Jul 22 2016, 11:16 AM
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

cfang added inline comments.Jul 28 2016, 4:53 PM
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.

arsenm added inline comments.Jul 28 2016, 4:56 PM
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

arsenm accepted this revision.Mar 3 2017, 8:20 PM

LGTM, I need this for other related fixes. This doesn't really solve the underlying issue here though

This revision is now accepted and ready to land.Mar 3 2017, 8:20 PM
cfang added a comment.Mar 7 2017, 3:05 PM

LGTM, I need this for other related fixes. This doesn't really solve the underlying issue here though

I still have a new LIT failure on ret_jump.ll to fix!

This revision was automatically updated to reflect the committed changes.