Page MenuHomePhabricator

AMDGPU: Fix an interaction between WQM and polygon stippling
ClosedPublic

Authored by nhaehnle on Aug 3 2016, 12:55 PM.

Details

Summary

This fixes a rare bug in polygon stippling with non-monolithic pixel shaders.

The underlying problem is as follows: the prolog part contains the polygon
stippling sequence, i.e. a kill. The main part then enables WQM based on the
_reduced_ exec mask, effectively undoing most of the polygon stippling.

Since we cannot know whether polygon stippling will be used, the main part
of a non-monolithic shader must always return to exact mode to fix this
problem.

Diff Detail

Repository
rL LLVM

Event Timeline

nhaehnle updated this revision to Diff 66697.Aug 3 2016, 12:55 PM
nhaehnle retitled this revision from to AMDGPU: Fix an interaction between WQM and polygon stippling.
nhaehnle updated this object.
nhaehnle added a subscriber: llvm-commits.
arsenm added inline comments.Aug 3 2016, 12:59 PM
lib/Target/AMDGPU/SIWholeQuadMode.cpp
224–225 ↗(On Diff #66697)

This seems to be checking that this is a return block. This will be wrong if the block ends in unreachable. Can you add a test with this?

nhaehnle updated this revision to Diff 66777.Aug 4 2016, 2:36 AM

Re-thinking the approach: I think it's best to just say that SI_RETURN
requires a switch to exact, since SI_RETURN is what marks the return in
non-void returning shaders. Also added an unreachable test.

arsenm added inline comments.Aug 10 2016, 5:02 PM
test/CodeGen/AMDGPU/wqm.ll
451–452 ↗(On Diff #66777)

You should maybe add some volatile stores or something here to make sure nothing tries to delete the block before

nhaehnle updated this revision to Diff 67661.Aug 11 2016, 2:06 AM
nhaehnle marked an inline comment as done.

Add a volatile store in the unreachable part of the test.

arsenm accepted this revision.Sep 1 2016, 10:16 AM
arsenm edited edge metadata.

LGTM

test/CodeGen/AMDGPU/wqm.ll
451–456 ↗(On Diff #67661)

I was thinking the function would only end in unreachable instead of having ret + unreachable. Both might be useful too, but one that has no ret and just unreachable would be good too

This revision is now accepted and ready to land.Sep 1 2016, 10:16 AM
This revision was automatically updated to reflect the committed changes.
calexil reopened this revision.Jan 9 2017, 3:27 PM

this commit breaks color rendering for amdgpu/radeon users and causes icons/etc to appear oversaturated red.

https://u.teknik.io/lqpTR.png

please revert or revise this

This revision is now accepted and ready to land.Jan 9 2017, 3:27 PM

this commit breaks color rendering for amdgpu/radeon users and causes icons/etc to appear oversaturated red.

https://u.teknik.io/lqpTR.png

please revert or revise this

Things seem to work fine with current trunk though, so this would probably only need to be reverted from the 3.9 branch, if no other solution can be found for that.

this commit breaks color rendering for amdgpu/radeon users and causes icons/etc to appear oversaturated red.

https://u.teknik.io/lqpTR.png

please revert or revise this

Things seem to work fine with current trunk though, so this would probably only need to be reverted from the 3.9 branch, if no other solution can be found for that.

not sure how to get oibaf to do that, since pkppa pulls llvm from there... I guess let him know it broke stuff?

nhaehnle closed this revision.Jul 14 2017, 6:55 AM

This was submitted long time ago.