As mentioned in D101115 this is a legacy pass that probably should not
be run at all, but definitely should not be run as part of the backend
codegen pass pipeline.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Frontends might need to start running CodeSinking as part of their optimization pass pipelines to avoid code quality regressions. For LLPC this is being done here: https://github.com/GPUOpen-Drivers/llpc/pull/1902
@mareko does Mesa need something similar?
Most of the codegen diffs come from the fact that CodeSinking modified IR that was generated by CodeGenPrepare.
llvm/test/CodeGen/AMDGPU/GlobalISel/atomic_optimizations_mul_one.ll | ||
---|---|---|
34 | This is an improvement - not sinking the popcnt that uses the result of a ballot means that this instruction can read from exec directly, instead of having to copy it to another sgpr. | |
llvm/test/CodeGen/AMDGPU/non-entry-alloca.ll | ||
97 | I don't think this test was testing the right thing. CodeSinking moved the alloca into bb.1 so the use was no longer "outside of the defining block". |
It was added in 2013 without much explanation: https://github.com/llvm/llvm-project/commit/4ee6dd61365ab746baf6c8f2be6507b08cd5c2da
Recently we've seen cases where it causes large increases in register pressure so we'd like to remove it altogether.
Whoops, I forgot that there are more tests failing that I haven't decided what to do with yet:
Failed Tests (5): LLVM :: CodeGen/AMDGPU/cgp-addressing-modes.ll LLVM :: CodeGen/AMDGPU/frame-index-elimination.ll LLVM :: CodeGen/AMDGPU/select-opt.ll LLVM :: CodeGen/AMDGPU/sink-image-sample.ll LLVM :: CodeGen/AMDGPU/valu-i1.ll
OK. I saw comments in D101115 saying "Mesa also needs it here", and adding a Mesa-specific test for sinking image sample instructions: test/CodeGen/AMDGPU/sink-image-sample.ll
When I added D101115 the expectation was that Mesa would need that change, but I do not know the current status. Worst case, we can add a flag to include the pass (off by default) and let front-ends opt in, but it would be really good if we could avoid the extra complexity.
I think we should drop the sinking pass from here. Worst case, we learn why it was added in the first place and figure things out from there.
llvm/test/CodeGen/AMDGPU/cgp-addressing-modes.ll | ||
---|---|---|
40 ↗ | (On Diff #446468) | Most of the tests in this file seem to be testing that we sink a getelementptr instruction into the same block as the load/store that uses it. I simply removed all the cases that fail. |
llvm/test/CodeGen/AMDGPU/sink-image-sample.ll | ||
4 ↗ | (On Diff #446468) | This is no longer done by the codegen pipeline, by design. Instead of removing this test I suppose we could change the RUN line to do "opt -sink" or "opt -sink | llc"? |
llvm/test/CodeGen/AMDGPU/sink-image-sample.ll | ||
---|---|---|
4 ↗ | (On Diff #446468) | Yes, that would make sense to me. |
I think we added backend passes in the past to compensate for mesa not running passes. It might need to start running it?
This is an improvement - not sinking the popcnt that uses the result of a ballot means that this instruction can read from exec directly, instead of having to copy it to another sgpr.