AMDGPU: Fix an interaction between WQM and polygon stippling

[nhaehnle]

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.

[arsenm]

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]

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.

[nhaehnle]

ping

[arsenm]

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]

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

[nhaehnle]

Ping

[arsenm]

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

[• calexil]

Revert this please:

https://www.reddit.com/r/linux_gaming/comments/5mt93d/anyone_else_having_issues_with_color_rendering/

[• calexil]

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

[michel.daenzer]

In D23131#640613, @calexil wrote:

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.

[• calexil]

In D23131#640968, @michel.daenzer wrote:

In D23131#640613, @calexil wrote:

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]

This was submitted long time ago.

llvm/trunk/lib/Target/AMDGPU/SIInstructions.td

 
 def SI_RETURN : SPseudoInstSI <
   (outs), (ins variable_ops), [(AMDGPUreturn)]> {
   let isTerminator = 1;
   let isBarrier = 1;
   let isReturn = 1;
   let hasSideEffects = 1;
   let hasNoSchedulingInfo = 1;
+  let DisableWQM = 1;
 }
 
 let Defs = [M0, EXEC],
   UseNamedOperandTable = 1 in {
 
 class SI_INDIRECT_SRC<RegisterClass rc> : VPseudoInstSI <
   (outs VGPR_32:$vdst),
   (ins rc:$src, VS_32:$idx, i32imm:$offset)> {

llvm/trunk/lib/Target/AMDGPU/SIWholeQuadMode.cpp

for (auto II = MBB.begin(), IE = MBB.end(); II != IE; ++II) {
 
         if (!Flags)
           continue;
       }
 
       markInstruction(MI, Flags, Worklist);
       GlobalFlags |= Flags;
     }
-
-    if (WQMOutputs && MBB.succ_empty()) {
-      // This is a prolog shader. Make sure we go back to exact mode at the end.
-      Blocks[&MBB].OutNeeds = StateExact;
-      Worklist.push_back(&MBB);
-      GlobalFlags |= StateExact;
-    }
   }
 
   return GlobalFlags;
 }
 
 void SIWholeQuadMode::propagateInstruction(MachineInstr &MI,
                                            std::vector<WorkItem>& Worklist) {
   MachineBasicBlock *MBB = MI.getParent();

llvm/trunk/test/CodeGen/AMDGPU/wqm.ll

main_body:
   ret <4 x float> %tex
 }
 
 ; Check that WQM is triggered by image samples and left untouched for loads...
 ;
 ;CHECK-LABEL: {{^}}test2:
 ;CHECK-NEXT: ; %main_body
 ;CHECK-NEXT: s_wqm_b64 exec, exec
-;CHECK: image_sample
 ;CHECK-NOT: exec
-;CHECK: _load_dword v0,
-define amdgpu_ps float @test2(<8 x i32> inreg %rsrc, <4 x i32> inreg %sampler, float addrspace(1)* inreg %ptr, <4 x i32> %c) {
+define amdgpu_ps void @test2(<8 x i32> inreg %rsrc, <4 x i32> inreg %sampler, float addrspace(1)* inreg %ptr, <4 x i32> %c) {
 main_body:
   %c.1 = call <4 x float> @llvm.SI.image.sample.v4i32(<4 x i32> %c, <8 x i32> %rsrc, <4 x i32> %sampler, i32 15, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0)
   %c.2 = bitcast <4 x float> %c.1 to <4 x i32>
   %c.3 = extractelement <4 x i32> %c.2, i32 0
   %gep = getelementptr float, float addrspace(1)* %ptr, i32 %c.3
   %data = load float, float addrspace(1)* %gep
-  ret float %data
+
+  call void @llvm.SI.export(i32 15, i32 1, i32 1, i32 0, i32 1, float %data, float undef, float undef, float undef)
+
+  ret void
 }
 
 ; ... but disabled for stores (and, in this simple case, not re-enabled).
 ;
 ;CHECK-LABEL: {{^}}test3:
 ;CHECK-NEXT: ; %main_body
 ;CHECK-NEXT: s_mov_b64 [[ORIG:s\[[0-9]+:[0-9]+\]]], exec
 ;CHECK-NEXT: s_wqm_b64 exec, exec
entry:
 
   %t = call <4 x float> @llvm.SI.image.sample.i32(i32 %c, <8 x i32> undef, <4 x i32> undef, i32 15, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0)
 
   call void @llvm.amdgcn.buffer.store.v4f32(<4 x float> %t, <4 x i32> undef, i32 0, i32 0, i1 0, i1 0)
 
   ret void
 }
 
+; Must return to exact at the end of a non-void returning shader,
+; otherwise the EXEC mask exported by the epilog will be wrong. This is true
+; even if the shader has no kills, because a kill could have happened in a
+; previous shader fragment.
+;
+; CHECK-LABEL: {{^}}test_nonvoid_return:
+; CHECK: s_mov_b64 [[LIVE:s\[[0-9]+:[0-9]+\]]], exec
+; CHECK: s_wqm_b64 exec, exec
+;
+; CHECK: s_and_b64 exec, exec, [[LIVE]]
+; CHECK-NOT: exec
+define amdgpu_ps <4 x float> @test_nonvoid_return() nounwind {
+  %tex = call <4 x float> @llvm.SI.image.sample.v4i32(<4 x i32> undef, <8 x i32> undef, <4 x i32> undef, i32 15, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0)
+  %tex.i = bitcast <4 x float> %tex to <4 x i32>
+  %dtex = call <4 x float> @llvm.SI.image.sample.v4i32(<4 x i32> %tex.i, <8 x i32> undef, <4 x i32> undef, i32 15, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0)
+  ret <4 x float> %dtex
+}
+
+; CHECK-LABEL: {{^}}test_nonvoid_return_unreachable:
+; CHECK: s_mov_b64 [[LIVE:s\[[0-9]+:[0-9]+\]]], exec
+; CHECK: s_wqm_b64 exec, exec
+;
+; CHECK: s_and_b64 exec, exec, [[LIVE]]
+; CHECK-NOT: exec
+define amdgpu_ps <4 x float> @test_nonvoid_return_unreachable(i32 inreg %c) nounwind {
+entry:
+  %tex = call <4 x float> @llvm.SI.image.sample.v4i32(<4 x i32> undef, <8 x i32> undef, <4 x i32> undef, i32 15, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0)
+  %tex.i = bitcast <4 x float> %tex to <4 x i32>
+  %dtex = call <4 x float> @llvm.SI.image.sample.v4i32(<4 x i32> %tex.i, <8 x i32> undef, <4 x i32> undef, i32 15, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0)
+
+  %cc = icmp sgt i32 %c, 0
+  br i1 %cc, label %if, label %else
+
+if:
+  store volatile <4 x float> %dtex, <4 x float>* undef
+  unreachable
+
+else:
+  ret <4 x float> %dtex
+}
 
 declare void @llvm.amdgcn.image.store.v4i32(<4 x float>, <4 x i32>, <8 x i32>, i32, i1, i1, i1, i1) #1
 declare void @llvm.amdgcn.buffer.store.f32(float, <4 x i32>, i32, i32, i1, i1) #1
 declare void @llvm.amdgcn.buffer.store.v4f32(<4 x float>, <4 x i32>, i32, i32, i1, i1) #1
 
 declare <4 x float> @llvm.amdgcn.image.load.v4i32(<4 x i32>, <8 x i32>, i32, i1, i1, i1, i1) #2
 declare float @llvm.amdgcn.buffer.load.f32(<4 x i32>, i32, i32, i1, i1) #2