Page MenuHomePhabricator

[AMDGPU] Fix the new atomic optimizer in pixel shaders.
ClosedPublic

Authored by sheredom on Oct 31 2018, 6:46 AM.

Details

Summary

The new atomic optimizer I previously added in D51969 did not work correctly when a pixel shader was using derivatives, and had helper lanes active.

To fix this we add an llvm.amdgcn.ps.live call that guards a branch around the entire atomic operation - ensuring that all helper lanes are inactive within the wavefront when we compute our atomic results.

I've added a test case that can cause derivatives, and exposes the problem.

Diff Detail

Repository
rL LLVM

Event Timeline

sheredom created this revision.Oct 31 2018, 6:46 AM
arsenm added inline comments.Oct 31 2018, 11:39 AM
test/CodeGen/AMDGPU/atomic_optimizations_pixelshader.ll
1 ↗(On Diff #171909)

Remove the -amdgiz's since that's long obsolete. Also remove -march since it's redundant with the triple

sheredom updated this revision to Diff 172104.Nov 1 2018, 3:25 AM

Review fixes.

sheredom marked an inline comment as done.Nov 1 2018, 3:26 AM
nhaehnle accepted this revision.Nov 5 2018, 3:02 AM
nhaehnle added inline comments.
test/CodeGen/AMDGPU/atomic_optimizations_pixelshader.ll
24 ↗(On Diff #171909)

Please remove the empty lines in this test and below.

41 ↗(On Diff #172104)

Why are there no check lines for GFX7LESS? What's the difference?

This revision is now accepted and ready to land.Nov 5 2018, 3:02 AM
sheredom updated this revision to Diff 172567.Nov 5 2018, 4:04 AM

Fix review comments made by @nhaehnle

sheredom marked 2 inline comments as done.Nov 5 2018, 4:05 AM
sheredom added inline comments.
test/CodeGen/AMDGPU/atomic_optimizations_pixelshader.ll
41 ↗(On Diff #172104)

The opt doesn't apply < gfx8 because of lack of cross-wave opts, but I should have checked that it definitely didn't apply (so I added the GFX7LESS-NOT lines).

This revision was automatically updated to reflect the committed changes.
sheredom marked an inline comment as done.