This condition seems unnecessary. The updates AMDGPU tests generally
have fewer instructions, but they are not particularly clear what they
intend to test.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 |
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:/ |
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 |
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
llvm/test/Transforms/Sink/single-succ.ll | ||
---|---|---|
1 | Can you generate this test, and do this on top of a baseline test commit? |
I think this was also to trick the dag, probably should add a conditional branch