This is an archive of the discontinued LLVM Phabricator instance.

[Sink] Process basic blocks with a single successor
ClosedPublic

Authored by MaskRay on Dec 17 2020, 9:29 PM.

Details

Summary

This condition seems unnecessary. The updates AMDGPU tests generally
have fewer instructions, but they are not particularly clear what they
intend to test.

Diff Detail

Event Timeline

MaskRay created this revision.Dec 17 2020, 9:29 PM
MaskRay requested review of this revision.Dec 17 2020, 9:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2020, 9:29 PM

Is there a reason to actually do this sinking? Normally the blocks would be merged anyway

llvm/test/CodeGen/AMDGPU/andorbitset.ll
57

I think this was also to trick the dag, probably should add a conditional branch

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.s.buffer.load.ll
213

I think this was intended to check whether the divergence was correctly propagated between blocks for %tmp. It would be better to add a branch condition to keep the shift in the entry block

Is there a reason to actually do this sinking? Normally the blocks would be merged anyway

As the llc tests show the blocks cannot always be merged. I think the main motivation is not "do sinking in this case", but rather "delete an unneeded condition".

llvm/test/CodeGen/AMDGPU/andorbitset.ll
57

The original test did not make it clear what s_bitset0_b32 tried to check, so I arbitrarily replaced it with another instruction in the entry basic block.

I don't know AMDGPU ISA and will need help to update this test and the test below:/

arsenm added inline comments.Dec 23 2020, 11:48 AM
llvm/test/CodeGen/AMDGPU/andorbitset.ll
57

I think the point of the test is the bitset needs to be there, so this is no longer checking the right thing

MaskRay updated this revision to Diff 313603.Dec 23 2020, 1:04 PM
MaskRay marked 3 inline comments as done.

AMDGPU tests: Add store to keep instructions in the entry block

Is there a reason to actually do this sinking? Normally the blocks would be merged anyway

As the llc tests show the blocks cannot always be merged. I think the main motivation is not "do sinking in this case", but rather "delete an unneeded condition".

This doesn't show that? This just shows testing artifacts. It's still legal to do the sinking. I'm just saying I don't see why this is advantageous over just letting the sink pass run with the expectation that simplifycfg cleaned up trivial branches earlier

arsenm requested changes to this revision.Nov 16 2022, 4:34 PM
arsenm added inline comments.
llvm/test/Transforms/Sink/single-succ.ll
1

Can you generate this test, and do this on top of a baseline test commit?

This revision now requires changes to proceed.Nov 16 2022, 4:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2022, 4:34 PM
arsenm accepted this revision.Nov 17 2022, 5:13 PM

LGTM. I can't see why this restriction would be here

This revision is now accepted and ready to land.Nov 17 2022, 5:13 PM
This revision was landed with ongoing or failed builds.Nov 17 2022, 5:23 PM
This revision was automatically updated to reflect the committed changes.