This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Stop running IR code sinking pass
AcceptedPublic

Authored by foad on Jul 20 2022, 6:57 AM.

Details

Reviewers
piotr
critson
nhaehnle
mareko
arsenm
Group Reviewers
Restricted Project
Summary

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.

Diff Detail

Event Timeline

foad created this revision.Jul 20 2022, 6:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2022, 6:57 AM
foad requested review of this revision.Jul 20 2022, 6:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2022, 6:57 AM
foad added a comment.Jul 20 2022, 7:05 AM

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".

There was a reason we did this but I don't remember the details

foad added a comment.Jul 20 2022, 7:30 AM

There was a reason we did this but I don't remember the details

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.

foad added a comment.Jul 20 2022, 8:55 AM

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

I think we don't need the pass, but it's hard to tell.

foad added a comment.Jul 21 2022, 1:49 AM

I think we don't need the pass, but it's hard to tell.

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

piotr added a comment.EditedJul 21 2022, 2:52 AM

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.

foad updated this revision to Diff 446468.Jul 21 2022, 6:30 AM

Fix remaining test failures

foad added inline comments.Jul 21 2022, 6:34 AM
llvm/test/CodeGen/AMDGPU/cgp-addressing-modes.ll
40

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

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"?

piotr added inline comments.Jul 21 2022, 7:47 AM
llvm/test/CodeGen/AMDGPU/sink-image-sample.ll
4

Yes, that would make sense to me.

foad updated this revision to Diff 446886.Jul 22 2022, 10:01 AM

Convert sink-image-sample.ll to run sink explicitly

foad marked an inline comment as done.Jul 22 2022, 10:02 AM

I think we added backend passes in the past to compensate for mesa not running passes. It might need to start running it?

arsenm accepted this revision.Nov 16 2022, 3:03 PM
This revision is now accepted and ready to land.Nov 16 2022, 3:03 PM

This can be merged. I have a patch for Mesa.

mareko accepted this revision.Dec 13 2022, 9:02 AM