[AMDGPU] Fix a miscompile with S_ADD/S_SUB

[piotr]

Summary

[AMDGPU] Fix a miscompile with S_ADD/S_SUB

The helper function isBoolSGPR is too aggressive when determining
when a v_cndmask can be skipped on a boolean value because the
function does not check the operands of and/or/xor.

This can be problematic for the Add/Sub combines that can leave
bits set even for inactive lanes leading to wrong results.

Fix this by inspecting the operands of and/or/xor recursively.

[arsenm]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

10255

It's only 32-bit for wave32. This also doesn't consider scalar booleans

10260–10264

I think this is the wrong place to fix this up. At this point we don't really know anything about lanes or registers, and the combine itself should already be correct

[piotr]

I think you are right - I moved the fix to the better place.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1056 ↗

(On Diff #291165)

I don't think it will end up mattering with the operations legal for i1 (although maybe trunc is also a problem), but I think it would be somewhat safer to list valid boolean sources instead

llvm/test/CodeGen/AMDGPU/combine-add-zext-xor.ll

17

Should be able to reduce this more

31–32

Should name values (opt -intsnamer)

60–61

Attributes canonically come after declarations

[piotr]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1056 ↗

(On Diff #291165)

Will do, but what do you mean exactly by "valid boolean sources" here?

llvm/test/CodeGen/AMDGPU/combine-add-zext-xor.ll

17

This already comes from bugpoint, but I will try to reduce the test a bit more.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1056 ↗

(On Diff #291165)

I mean like setcc + class intrinsics, other things that will select to VOPC outputs

llvm/test/CodeGen/AMDGPU/combine-add-zext-xor.ll

17

Bugpoint isn't that smart and you can usually massage the IR a bit to help it make more progress

[piotr]

Simplified test and added 5 more.

[piotr]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1056 ↗

(On Diff #291165)

I looked at this and I am not convinced we need to handle setcc here (added TODO). And fp_class would be mapped to VALU, so no need to handle this here either.

llvm/test/CodeGen/AMDGPU/combine-add-zext-xor.ll

17

Simplified test (also added "-start-before=amdgpu-isel -stop-after=amdgpu-isel") and added 5 more cases.

[piotr]

Ping.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1056 ↗

(On Diff #291165)

I mean entirely invert the logic to specifically check for the cases that will map to VOPC. You would not be adding setcc, you would be checking setcc and co. instead of and/or/xor

[piotr]

Ah now I see what you meant, sorry.

[piotr]

Invert the condition.

[foad]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1056 ↗

(On Diff #297352)

In future it would be nice to improve this to detect a tree of SETCCs connected by ANDs and ORs. This kind of pattern seems to occur quite frequently:

        v_cmp_ne_u32_e64 s29, s29, 1
        v_cmp_ne_u32_e64 s0, s0, 0
        s_and_b32 s0, s0, s29
+       s_and_b32 s0, exec_lo, s0

where your patch has added the last AND instruction.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1056 ↗

(On Diff #297352)

I thought we had an isVectorBoolean or something like that for identifying these cases recursively (I know I have a patch implementing the equivalent for globalisel lying around at least)

[piotr]

Good spot - I looked at the shader database and indeed the pattern with two connected setcc is frequent enough to special case it so I updated the patch.

[piotr]

Renamed variable to avoid a multiline assignment.

[foad]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1056 ↗

(On Diff #297352)

There's this in SIISelLowering.cpp:

// Returns true if argument is a boolean value which is not serialized into
// memory or argument and does not require v_cmdmask_b32 to be deserialized.
static bool isBoolSGPR(SDValue V) {

[piotr]

Ping.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1059–1063 ↗

(On Diff #298372)

Should reuse isBoolSGPR

[piotr]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1059–1063 ↗

(On Diff #298372)

isBoolSGPR() does not work for my case - I need to distinguish between two groups (AND/OR/XOR) and (SETCC/FP_CLASS), but that function returns true for both.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1059–1063 ↗

(On Diff #298372)

I'd say isBoolSGPR is buggy then. In any case, this should be moved out to a separate function

[foad]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1059–1063 ↗

(On Diff #298372)

Currently isBoolSGPR returns true for any and/or/xor, without recursively checking that the operands are boolean. But perhaps it should be fixed? Then you could use it here too.

[arsenm]

reverse ping

[piotr]

Sorry for the lack of activity lately - I plan to get back to this patch in the near future.

[piotr]

Following the suggesstions of updating isBoolSGPR.

[arsenm]

I think you only posted a partial patch for the latest revision

llvm/test/CodeGen/AMDGPU/combine-add-zext-xor.ll

1

Skipping all the IR passes and going straight to the isel is really weird (and I don't think actually works in general). Is there a need to skip the IR passes here?

Can you also stop after SIFixSGPRCopies (or finalizeisel) instead?

[piotr]

This is the intended patch, no need to change AMDGPUISelDAGToDAG.cpp in the most recent version. Here, due to the updated check in isBoolSGPR the problematic combines that would strip zext will not happen (see SITargetLowering::performAddCombine/performSubCombine). This is similar to the very first approach I put up for a review.

llvm/test/CodeGen/AMDGPU/combine-add-zext-xor.ll

1

Sure, will update the test.

[foad]

Patch LGTM modulo Matt's comments about the test.

[piotr]

Tests rewritten.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

const SIInstrInfo *TII = getSubtarget()->getInstrInfo();
     // materialization.
     return splitBinaryBitConstantOpImpl(DCI, SL, Opc, LHS, ValLo, ValHi);
   }
 
   return SDValue();
 }
 
 // Returns true if argument is a boolean value which is not serialized into
-// memory or argument and does not require v_cmdmask_b32 to be deserialized.
+// memory or argument and does not require v_cndmask_b32 to be deserialized.
 static bool isBoolSGPR(SDValue V) {
   if (V.getValueType() != MVT::i1)
     return false;
   switch (V.getOpcode()) {
-  default: break;
+  default:
+    break;
   case ISD::SETCC:
+  case AMDGPUISD::FP_CLASS:
+    return true;
   case ISD::AND:
   case ISD::OR:
   case ISD::XOR:
-  case AMDGPUISD::FP_CLASS:
-    return true;
+    return isBoolSGPR(V.getOperand(0)) && isBoolSGPR(V.getOperand(1));
   }
   return false;
 }
 
 // If a constant has all zeroes or all ones within each byte return it.
 // Otherwise return 0.
 static uint32_t getConstantPermuteMask(uint32_t C) {
   // 0xff for any zero byte in the mask
SDValue SITargetLowering::performAddCombine(SDNode *N,
   case ISD::SIGN_EXTEND:
   case ISD::ANY_EXTEND: {
     auto Cond = RHS.getOperand(0);
     // If this won't be a real VOPC output, we would still need to insert an
     // extra instruction anyway.
     if (!isBoolSGPR(Cond))
       break;
     SDVTList VTList = DAG.getVTList(MVT::i32, MVT::i1);
     SDValue Args[] = { LHS, DAG.getConstant(0, SL, MVT::i32), Cond };
     Opc = (Opc == ISD::SIGN_EXTEND) ? ISD::SUBCARRY : ISD::ADDCARRY;
     return DAG.getNode(Opc, SL, VTList, Args);
   }
   case ISD::ADDCARRY: {
     // add x, (addcarry y, 0, cc) => addcarry x, y, cc
     auto C = dyn_cast<ConstantSDNode>(RHS.getOperand(1));
     if (!C || C->getZExtValue() != 0) break;
     SDValue Args[] = { LHS, RHS.getOperand(0), RHS.getOperand(2) };
     return DAG.getNode(ISD::ADDCARRY, SDLoc(N), RHS->getVTList(), Args);
   }
   }
   return SDValue();
 }
 
 SDValue SITargetLowering::performSubCombine(SDNode *N,
                                             DAGCombinerInfo &DCI) const {
   SelectionDAG &DAG = DCI.DAG;

llvm/test/CodeGen/AMDGPU/combine-add-zext-xor.ll

+; RUN: llc -march=amdgcn -start-before=amdgpu-isel  -stop-after=amdgpu-isel -mtriple=amdgcn--amdpal -verify-machineinstrs < %s | FileCheck %s
+
+; Test that unused lanes in the s_xor result are masked out with v_cndmask.
+
+; CHECK-LABEL: name:            combine_add_zext_xor
+; CHECK: S_XOR_B64
+; CHECK-NEXT: V_CNDMASK
+; CHECK: S_ADD
+
+define void @combine_add_zext_xor() {
+.entry:
+  br label %.exit, !amdgpu.uniform !0
+
+.exit:                                            ; preds = %bb7, %.entry
+  %t = phi i32 [ 1050, %.entry ], [ 0, %bb7 ]
+  %t1 = phi i32 [ 0, %.entry ], [ %t7, %bb7 ]
+  br i1 undef, label %bb7, label %bb, !amdgpu.uniform !0
+
+bb:                                               ; preds = %.exit
+  %t2 = add i32 %t, 2
+  %t3 = icmp eq i32 %t2, 0
+  br label %bb7, !amdgpu.uniform !0
+
+bb7:                                              ; preds = %bb, %.exit
+  %t4 = phi i1 [ %t3, %bb ], [ undef, %.exit ]
+  %t5 = xor i1 %t4, true
+  %t6 = zext i1 %t5 to i32
+  %t7 = add i32 %t1, %t6
+  br label %.exit, !amdgpu.uniform !0
+}
+
+; Test that unused lanes in the s_and result are masked out with v_cndmask.
+
+; CHECK-LABEL: name:            combine_add_zext_and
+; CHECK: S_AND_B64
+; CHECK-NEXT: V_CNDMASK
+; CHECK: S_ADD
+
+define void @combine_add_zext_and() {
+.entry:
+  br label %.exit, !amdgpu.uniform !0
+
+.exit:                                            ; preds = %bb7, %.entry
+  %t = phi i32 [ 1050, %.entry ], [ 0, %bb7 ]
+  %t1 = phi i32 [ 0, %.entry ], [ %t7, %bb7 ]
+  br i1 undef, label %bb7, label %bb, !amdgpu.uniform !0
+
+bb:                                               ; preds = %.exit
+  %t2 = add i32 %t, 2
+  %t3 = icmp eq i32 %t2, 0
+  br label %bb7, !amdgpu.uniform !0
+
+bb7:                                              ; preds = %bb, %.exit
+  %t4 = phi i1 [ %t3, %bb ], [ undef, %.exit ]
+  %t4a = phi i1 [ %t3, %bb ], [ undef, %.exit ]
+  %t5 = and i1 %t4, %t4a
+  %t6 = zext i1 %t5 to i32
+  %t7 = add i32 %t1, %t6
+  br label %.exit, !amdgpu.uniform !0
+}
+
+; Test that unused lanes in the s_or result are masked out with v_cndmask.
+
+; CHECK-LABEL: name:            combine_add_zext_or
+; CHECK: S_OR_B64
+; CHECK-NEXT: V_CNDMASK
+; CHECK: S_ADD
+
+define void @combine_add_zext_or() {
+.entry:
+  br label %.exit, !amdgpu.uniform !0
+
+.exit:                                            ; preds = %bb7, %.entry
+  %t = phi i32 [ 1050, %.entry ], [ 0, %bb7 ]
+  %t1 = phi i32 [ 0, %.entry ], [ %t7, %bb7 ]
+  br i1 undef, label %bb7, label %bb, !amdgpu.uniform !0
+
+bb:                                               ; preds = %.exit
+  %t2 = add i32 %t, 2
+  %t3 = icmp eq i32 %t2, 0
+  br label %bb7, !amdgpu.uniform !0
+
+bb7:                                              ; preds = %bb, %.exit
+  %t4 = phi i1 [ %t3, %bb ], [ undef, %.exit ]
+  %t4a = phi i1 [ %t3, %bb ], [ undef, %.exit ]
+  %t5 = or i1 %t4, %t4a
+  %t6 = zext i1 %t5 to i32
+  %t7 = add i32 %t1, %t6
+  br label %.exit, !amdgpu.uniform !0
+}
+
+; Test that unused lanes in the s_xor result are masked out with v_cndmask.
+
+; CHECK-LABEL: name:            combine_sub_zext_xor
+; CHECK: S_XOR_B64
+; CHECK-NEXT: V_CNDMASK
+; CHECK: S_SUB
+
+define void @combine_sub_zext_xor() {
+.entry:
+  br label %.exit, !amdgpu.uniform !0
+
+.exit:                                            ; preds = %bb7, %.entry
+  %t = phi i32 [ 1050, %.entry ], [ 0, %bb7 ]
+  %t1 = phi i32 [ 0, %.entry ], [ %t7, %bb7 ]
+  br i1 undef, label %bb7, label %bb, !amdgpu.uniform !0
+
+bb:                                               ; preds = %.exit
+  %t2 = add i32 %t, 2
+  %t3 = icmp eq i32 %t2, 0
+  br label %bb7, !amdgpu.uniform !0
+
+bb7:                                              ; preds = %bb, %.exit
+  %t4 = phi i1 [ %t3, %bb ], [ undef, %.exit ]
+  %t5 = xor i1 %t4, true
+  %t6 = zext i1 %t5 to i32
+  %t7 = sub i32 %t1, %t6
+  br label %.exit, !amdgpu.uniform !0
+}
+
+; Test that unused lanes in the s_and result are masked out with v_cndmask.
+
+; CHECK-LABEL: name:            combine_sub_zext_and
+; CHECK: S_AND_B64
+; CHECK-NEXT: V_CNDMASK
+; CHECK: S_SUB
+
+define void @combine_sub_zext_and() {
+.entry:
+  br label %.exit, !amdgpu.uniform !0
+
+.exit:                                            ; preds = %bb7, %.entry
+  %t = phi i32 [ 1050, %.entry ], [ 0, %bb7 ]
+  %t1 = phi i32 [ 0, %.entry ], [ %t7, %bb7 ]
+  br i1 undef, label %bb7, label %bb, !amdgpu.uniform !0
+
+bb:                                               ; preds = %.exit
+  %t2 = add i32 %t, 2
+  %t3 = icmp eq i32 %t2, 0
+  br label %bb7, !amdgpu.uniform !0
+
+bb7:                                              ; preds = %bb, %.exit
+  %t4 = phi i1 [ %t3, %bb ], [ undef, %.exit ]
+  %t4a = phi i1 [ %t3, %bb ], [ undef, %.exit ]
+  %t5 = and i1 %t4, %t4a
+  %t6 = zext i1 %t5 to i32
+  %t7 = sub i32 %t1, %t6
+  br label %.exit, !amdgpu.uniform !0
+}
+
+; Test that unused lanes in the s_or result are masked out with v_cndmask.
+
+; CHECK-LABEL: name:            combine_sub_zext_or
+; CHECK: S_OR_B64
+; CHECK-NEXT: V_CNDMASK
+; CHECK: S_SUB
+
+define void @combine_sub_zext_or() {
+.entry:
+  br label %.exit, !amdgpu.uniform !0
+
+.exit:                                            ; preds = %bb7, %.entry
+  %t = phi i32 [ 1050, %.entry ], [ 0, %bb7 ]
+  %t1 = phi i32 [ 0, %.entry ], [ %t7, %bb7 ]
+  br i1 undef, label %bb7, label %bb, !amdgpu.uniform !0
+
+bb:                                               ; preds = %.exit
+  %t2 = add i32 %t, 2
+  %t3 = icmp eq i32 %t2, 0
+  br label %bb7, !amdgpu.uniform !0
+
+bb7:                                              ; preds = %bb, %.exit
+  %t4 = phi i1 [ %t3, %bb ], [ undef, %.exit ]
+  %t4a = phi i1 [ %t3, %bb ], [ undef, %.exit ]
+  %t5 = or i1 %t4, %t4a
+  %t6 = zext i1 %t5 to i32
+  %t7 = sub i32 %t1, %t6
+  br label %.exit, !amdgpu.uniform !0
+}
+
+attributes #0 = { nounwind readonly willreturn }
+
+!0 = !{}