[AMDGPU] Fixed incorrect uniform branch condition

[tpr]

Summary

I had a case where multiple nested uniform ifs resulted in code that did
v_cmp comparisons, combining the results with s_and_b64, s_or_b64 and
s_xor_b64 and using the resulting mask in s_cbranch_vccnz, without first
ensuring that bits for inactive lanes were clear.

There was already code for inserting an "s_and_b64 vcc, exec, vcc" to
clear bits for inactive lanes in the case that the branch is instruction
selected as s_cbranch_scc1 and is then changed to s_cbranch_vccnz in
SIFixSGPRCopies. I have moved that code into SILowerControlFlow so it
also handles the case that the branch is instruction selected as
s_cbranch_vccnz.

I have also added some code to analyze whether the s_and_b64 needs to be
inserted at all. If vcc was defined by a v_cmp, or a combination via
s_and/s_or of a number of v_cmp instructions, then it is not needed at
all. The s_and_b64 is now not inserted in these cases.

Several tests had such an unnecessary s_and_b64, so needed modifying to
cope with its disappearance. The indirect-addressing-si.ll test also
seemed to be affected by some kind of loop transformation, so the lit
script needed changing to cope with that.

[arsenm]

The clearing extra clearing when moving branch on scc was a leftover that should have been removed in r286134. I also don't understand why it would be needed in that case, since the moved condition is going to be a V_CMP* that does the right thing for inactive lanes.

I think this is backwards. We shouldn't be looking at the already lowered control flow instructions and trying to insert semantically required instructions. This pass is specifically for expanding the exec mask modifications, not general control flow. It could perhaps be renamed. I think you should partially revert r286134 unless the condition source is known to be a compare.

An optimisation pass should then try to eliminate the unnecessary ands, perhaps SIOptimizeExecMaskingPreRA

[tpr]

Ah, I didn't see r286134.

How do you mean, partially revert it? It looks like I want to completely revert it and fix the comment for semantic correctness, then think about optimizing the cases when the s_and is not needed.

[arsenm]

In D41292#956918, @tpr wrote:

Ah, I didn't see r286134.

How do you mean, partially revert it? It looks like I want to completely revert it and fix the comment for semantic correctness, then think about optimizing the cases when the s_and is not needed.

I mean mostly keep it, just re-insert the and when the branch condition isn't known to be a compare like it implicitly assumes

[arsenm]

In D41292#956933, @arsenm wrote:

In D41292#956918, @tpr wrote:

Ah, I didn't see r286134.

How do you mean, partially revert it? It looks like I want to completely revert it and fix the comment for semantic correctness, then think about optimizing the cases when the s_and is not needed.

I mean mostly keep it, just re-insert the and when the branch condition isn't known to be a compare like it implicitly assumes

I suppose there are a few other special cases, like CMP_CLASS and other known-to-be-i1 produced instructions

[tpr]

V2: Always add the s_and. We should add a later pass to remove it when it is not needed.

[tpr]

If there are no further comments, I'll land this.

[arsenm]

lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1680 ↗

(On Diff #127258)

Can you add further explanation? Should it still be OK as long as the incoming condition can be seen to be a VCC producer / not a bit op?

[tpr]

Expanded comment.

[arsenm]

LGTM. Are you planning on doing the optimization soon?

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

if (Cond.isUndef()) {
     return;
   }
 
   bool UseSCCBr = isCBranchSCC(N) && isUniformBr(N);
   unsigned BrOp = UseSCCBr ? AMDGPU::S_CBRANCH_SCC1 : AMDGPU::S_CBRANCH_VCCNZ;
   unsigned CondReg = UseSCCBr ? AMDGPU::SCC : AMDGPU::VCC;
   SDLoc SL(N);
 
+  if (!UseSCCBr) {
+    // This is the case that we are selecting to S_CBRANCH_VCCNZ.  We have not
+    // analyzed what generates the vcc value, so we do not know whether vcc
+    // bits for disabled lanes are 0.  Thus we need to mask out bits for
+    // disabled lanes.
+    //
+    // For the case that we select S_CBRANCH_SCC1 and it gets
+    // changed to S_CBRANCH_VCCNZ in SIFixSGPRCopies, SIFixSGPRCopies calls
+    // SIInstrInfo::moveToVALU which inserts the S_AND).
+    //
+    // We could add an analysis of what generates the vcc value here and omit
+    // the S_AND when is unnecessary. But it would be better to add a separate
+    // pass after SIFixSGPRCopies to do the unnecessary S_AND removal, so it
+    // catches both cases.
+    Cond = SDValue(CurDAG->getMachineNode(AMDGPU::S_AND_B64, SL, MVT::i1,
+                               CurDAG->getRegister(AMDGPU::EXEC, MVT::i1),
+                               Cond),
+                   0);
+  }
+
   SDValue VCC = CurDAG->getCopyToReg(N->getOperand(0), SL, CondReg, Cond);
   CurDAG->SelectNodeTo(N, BrOp, MVT::Other,
                        N->getOperand(2), // Basic Block
                        VCC.getValue(0));
 }
 
 void AMDGPUDAGToDAGISel::SelectFMAD(SDNode *N) {
   MVT VT = N->getSimpleValueType(0);

llvm/trunk/test/CodeGen/AMDGPU/branch-relaxation.ll

 bb3:
   store volatile i32 %cnd, i32 addrspace(1)* %arg
   ret void
 }
 
 ; GCN-LABEL: {{^}}uniform_conditional_min_long_forward_vcnd_branch:
 ; GCN: s_load_dword [[CND:s[0-9]+]]
 ; GCN-DAG: v_mov_b32_e32 [[V_CND:v[0-9]+]], [[CND]]
-; GCN-DAG: v_cmp_eq_f32_e64 vcc, [[CND]], 0
+; GCN-DAG: v_cmp_eq_f32_e64 [[UNMASKED:s\[[0-9]+:[0-9]+\]]], [[CND]], 0
+; GCN-DAG: s_and_b64 vcc, exec, [[UNMASKED]]
 ; GCN: s_cbranch_vccz [[LONGBB:BB[0-9]+_[0-9]+]]
 
 ; GCN-NEXT: [[LONG_JUMP:BB[0-9]+_[0-9]+]]: ; %bb0
 ; GCN-NEXT: s_getpc_b64 vcc
 ; GCN-NEXT: s_add_u32 vcc_lo, vcc_lo, [[ENDBB:BB[0-9]+_[0-9]+]]-([[LONG_JUMP]]+4)
 ; GCN-NEXT: s_addc_u32 vcc_hi, vcc_hi, 0
 ; GCN-NEXT: s_setpc_b64 vcc
 

 ; GCN-DAG: v_cmp_lt_i32
 ; GCN-DAG: v_cmp_gt_i32
 ; GCN: s_cbranch_vccnz
 
 ; GCN: s_setpc_b64
 ; GCN: s_setpc_b64
 
 ; GCN: [[LONG_BR_DEST0]]
-; GCN: v_cmp_ne_u32_e32
-; GCN-NEXT: s_cbranch_vccz
+; GCN: s_cbranch_vccz
 ; GCN: s_setpc_b64
 
 ; GCN: s_endpgm
 define amdgpu_kernel void @long_branch_hang(i32 addrspace(1)* nocapture %arg, i32 %arg1, i32 %arg2, i32 %arg3, i32 %arg4, i64 %arg5) #0 {
 bb:
   %tmp = icmp slt i32 %arg2, 9
   %tmp6 = icmp eq i32 %arg1, 0
   %tmp7 = icmp sgt i32 %arg4, 0
   %tmp8 = icmp sgt i32 %arg4, 5
   br i1 %tmp8, label %bb9, label %bb13
 
 bb9:                                              ; preds = %bb
   %tmp10 = and i1 %tmp7, %tmp
   %tmp11 = icmp slt i32 %arg3, %arg4
   %tmp12 = or i1 %tmp11, %tmp7
   br i1 %tmp12, label %bb19, label %bb14
 
 bb13:                                             ; preds = %bb
+  call void asm sideeffect
+  "v_nop_e64
+   v_nop_e64
+   v_nop_e64
+   v_nop_e64", ""() #0
   br i1 %tmp6, label %bb19, label %bb14
 
 bb14:                                             ; preds = %bb13, %bb9
   %tmp15 = icmp slt i32 %arg3, %arg4
   %tmp16 = or i1 %tmp15, %tmp
   %tmp17 = and i1 %tmp6, %tmp16
   %tmp18 = zext i1 %tmp17 to i32
   br label %bb19

llvm/trunk/test/CodeGen/AMDGPU/cf-loop-on-constant.ll

for.body:
   %add = fadd float %vecload, 1.0
   store float %add, float addrspace(3)* %arrayidx, align 8
   %inc = add i32 %indvar, 1
   br i1 undef, label %for.body, label %for.exit
 }
 
 ; GCN-LABEL: {{^}}loop_arg_0:
 ; GCN: v_and_b32_e32 v{{[0-9]+}}, 1, v{{[0-9]+}}
-; GCN: v_cmp_eq_u32_e32 vcc, 1,
+; GCN: v_cmp_eq_u32{{[^,]*}}, 1,
 
 ; GCN: [[LOOPBB:BB[0-9]+_[0-9]+]]
 ; GCN: s_add_i32 s{{[0-9]+}}, s{{[0-9]+}}, 0x80
 ; GCN: s_add_i32 s{{[0-9]+}}, s{{[0-9]+}}, 4
 
 ; GCN: s_cbranch_vccnz [[LOOPBB]]
 ; GCN-NEXT: ; %bb.2
 ; GCN-NEXT: s_endpgm

llvm/trunk/test/CodeGen/AMDGPU/nested-loop-conditions.ll

 ; IR-NEXT: ret void
 
 ; GCN-LABEL: {{^}}reduced_nested_loop_conditions:
 
 ; GCN: s_cmp_eq_u32 s{{[0-9]+}}, 1
 ; GCN-NEXT: s_cbranch_scc1
 
 ; FIXME: Should fold to unconditional branch?
-; GCN: s_mov_b64 vcc, -1
-; GCN-NEXT: ; implicit-def
+; GCN: ; implicit-def
 ; GCN: s_cbranch_vccz
 
 ; GCN: ds_read_b32
 
 ; GCN: [[BB9:BB[0-9]+_[0-9]+]]: ; %bb9
 ; GCN-NEXT: ; =>This Inner Loop Header: Depth=1
 ; GCN-NEXT: s_branch [[BB9]]
 define amdgpu_kernel void @reduced_nested_loop_conditions(i64 addrspace(3)* nocapture %arg) #0 {

llvm/trunk/test/CodeGen/AMDGPU/scalar-branch-missing-and-exec.ll

+; RUN: llc -march=amdgcn -mcpu=gfx600 -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -march=amdgcn -mcpu=gfx700 -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -march=amdgcn -mcpu=gfx800 -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck %s
+
+; This checks for a bug where uniform control flow can result in multiple
+; v_cmp results being combined together with s_and_b64, s_or_b64 and s_xor_b64,
+; using the resulting mask in s_cbranch_vccnz
+; without ensuring that the resulting mask has bits clear for inactive lanes.
+; The problematic case is s_xor_b64, as, unlike the other ops, it can actually
+; set bits for inactive lanes.
+;
+; The check for an s_xor_b64 is just to check that this test tests what it is
+; supposed to test. If the s_xor_b64 disappears due to some other case, it does
+; not necessarily mean that the bug has reappeared.
+;
+; The check for "s_and_b64 vcc, exec, something" checks that the bug is fixed.
+
+; CHECK: {{^}}main:
+; CHECK: s_xor_b64
+; CHECK: s_and_b64 vcc, exec,
+
+define amdgpu_cs void @main(i32 inreg %arg) {
+.entry:
+  %tmp44 = load volatile <2 x float>, <2 x float> addrspace(1)* undef
+  %tmp16 = load volatile float, float addrspace(1)* undef
+  %tmp22 = load volatile float, float addrspace(1)* undef
+  %tmp25 = load volatile float, float addrspace(1)* undef
+  %tmp31 = fcmp olt float %tmp16, 0x3FA99999A0000000
+  br i1 %tmp31, label %bb, label %.exit.thread
+
+bb:                                               ; preds = %.entry
+  %tmp42 = fcmp olt float %tmp25, 0x3FA99999A0000000
+  br i1 %tmp42, label %bb43, label %.exit.thread
+
+bb43:
+  %tmp46 = fcmp olt <2 x float> %tmp44, <float 0x3FA99999A0000000, float 0x3FA99999A0000000>
+  %tmp47 = extractelement <2 x i1> %tmp46, i32 0
+  %tmp48 = extractelement <2 x i1> %tmp46, i32 1
+  %tmp49 = and i1 %tmp47, %tmp48
+  br i1 %tmp49, label %bb50, label %.exit.thread
+
+bb50:
+  %tmp53 = fcmp olt float %tmp22, 0x3FA99999A0000000
+  br i1 %tmp53, label %.exit3.i, label %.exit.thread
+
+.exit3.i:
+  store volatile i32 0, i32 addrspace(1)* undef
+  br label %.exit.thread
+
+.exit.thread:
+  ret void
+}
+

llvm/trunk/test/CodeGen/AMDGPU/select-opt.ll

define amdgpu_kernel void @opt_select_i64_or_cmp_f32(i64 addrspace(1)* %out, float %a, float %b, float %c, i64 %x, i64 %y) #0 {
   %fcmp1 = fcmp one float %a, %c
   %or = or i1 %fcmp0, %fcmp1
   %select = select i1 %or, i64 %x, i64 %y
   store i64 %select, i64 addrspace(1)* %out
   ret void
 }
 
 ; GCN-LABEL: {{^}}regression:
-; GCN: v_cmp_neq_f32_e64 vcc
-; GCN: v_cmp_neq_f32_e64 vcc, s{{[0-9]+}}, 0
+; GCN: v_cmp_neq_f32_e64
+; GCN: v_cmp_neq_f32_e64 {{[^,]*}}, s{{[0-9]+}}, 0
 ; GCN: v_cmp_ne_u32_e32 vcc, 0, v{{[0-9]+}}
 
 define amdgpu_kernel void @regression(float addrspace(1)* %out, float %c0, float %c1) #0 {
 entry:
   %cmp0 = fcmp oeq float %c0, 1.0
   br i1 %cmp0, label %if0, label %endif
 
 if0:

llvm/trunk/test/CodeGen/AMDGPU/skip-if-dead.ll

 ; CHECK: exp
 ; CHECK-NEXT: s_endpgm
 
 ; CHECK: [[KILLBB:BB[0-9]+_[0-9]+]]:
 ; CHECK-NEXT: s_cbranch_scc0 [[PHIBB:BB[0-9]+_[0-9]+]]
 
 ; CHECK: [[PHIBB]]:
 ; CHECK: v_cmp_eq_f32_e32 vcc, 0, [[PHIREG]]
-; CHECK-NEXT: s_cbranch_vccz [[ENDBB:BB[0-9]+_[0-9]+]]
+; CHECK: s_cbranch_vccz [[ENDBB:BB[0-9]+_[0-9]+]]
 
 ; CHECK: ; %bb10
 ; CHECK: v_mov_b32_e32 v{{[0-9]+}}, 9
 ; CHECK: buffer_store_dword
 
 ; CHECK: [[ENDBB]]:
 ; CHECK-NEXT: s_endpgm
 define amdgpu_ps void @phi_use_def_before_kill() #0 {
bb10:
   br label %end
 
 end:
   ret void
 }
 
 ; CHECK-LABEL: {{^}}no_skip_no_successors:
 ; CHECK: v_cmp_nge_f32
-; CHECK-NEXT: s_cbranch_vccz [[SKIPKILL:BB[0-9]+_[0-9]+]]
+; CHECK: s_cbranch_vccz [[SKIPKILL:BB[0-9]+_[0-9]+]]
 
 ; CHECK: ; %bb6
 ; CHECK: s_mov_b64 exec, 0
 
 ; CHECK: [[SKIPKILL]]:
 ; CHECK: v_cmp_nge_f32_e32 vcc
-; CHECK-NEXT: %bb.3: ; %bb5
+; CHECK: %bb.3: ; %bb5
 ; CHECK-NEXT: .Lfunc_end{{[0-9]+}}
 define amdgpu_ps void @no_skip_no_successors(float inreg %arg, float inreg %arg1) #0 {
 bb:
   %tmp = fcmp ult float %arg1, 0.000000e+00
   %tmp2 = fcmp ult float %arg, 0x3FCF5C2900000000
   br i1 %tmp, label %bb6, label %bb3
 
 bb3:                                              ; preds = %bb

llvm/trunk/test/CodeGen/AMDGPU/smrd-vccz-bug.ll

 ; RUN: llc -march=amdgcn -mcpu=verde -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=VCCZ-BUG %s
 ; RUN: llc -march=amdgcn -mcpu=bonaire -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=VCCZ-BUG %s
 ; RUN: llc -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=GCN -check-prefix=NOVCCZ-BUG %s
 
 ; GCN-FUNC: {{^}}vccz_workaround:
 ; GCN: s_load_dword s{{[0-9]+}}, s[{{[0-9]+:[0-9]+}}], 0x0
-; GCN: v_cmp_neq_f32_e64 vcc, s{{[0-9]+}}, 0{{$}}
+; GCN: v_cmp_neq_f32_e64 {{[^,]*}}, s{{[0-9]+}}, 0{{$}}
 ; VCCZ-BUG: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; VCCZ-BUG: s_mov_b64 vcc, vcc
 ; NOVCCZ-BUG-NOT: s_mov_b64 vcc, vcc
 ; GCN: s_cbranch_vccnz [[EXIT:[0-9A-Za-z_]+]]
 ; GCN: buffer_store_dword
 ; GCN: [[EXIT]]:
 ; GCN: s_endpgm
 define amdgpu_kernel void @vccz_workaround(i32 addrspace(2)* %in, i32 addrspace(1)* %out, float %cond) {

llvm/trunk/test/CodeGen/AMDGPU/uniform-cfg.ll

 ENDIF:                                            ; preds = %IF, %main_body
   ret void
 }
 
 ; GCN-LABEL: {{^}}icmp_users_different_blocks:
 ; GCN: s_load_dword [[COND:s[0-9]+]]
 ; GCN: s_cmp_lt_i32 [[COND]], 1
 ; GCN: s_cbranch_scc1 [[EXIT:[A-Za-z0-9_]+]]
-; GCN: v_cmp_gt_i32_e64 vcc, [[COND]], 0{{$}}
+; GCN: v_cmp_gt_i32_e64 {{[^,]*}}, [[COND]], 0{{$}}
 ; GCN: s_cbranch_vccz [[BODY:[A-Za-z0-9_]+]]
 ; GCN: {{^}}[[EXIT]]:
 ; GCN: s_endpgm
 ; GCN: {{^}}[[BODY]]:
 ; GCN: buffer_store
 ; GCN: s_endpgm
 define amdgpu_kernel void @icmp_users_different_blocks(i32 %cond0, i32 %cond1, i32 addrspace(1)* %out) {
 bb: