[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

10123 ↗

(On Diff #288955)

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

10128–10132 ↗

(On Diff #288955)

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

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

16

Should be able to reduce this more

30–31

Should name values (opt -intsnamer)

59–60

Attributes canonically come after declarations

[piotr]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1056

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

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

16

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

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1056

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

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

16

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

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

16

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

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

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

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

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

Should reuse isBoolSGPR

[piotr]

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

1059–1063

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

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

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

2

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

2

Sure, will update the test.

[foad]

Patch LGTM modulo Matt's comments about the test.

[piotr]

Tests rewritten.

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

void AMDGPUDAGToDAGISel::SelectAddcSubb(SDNode *N) {
   if (N->isDivergent()) {
     unsigned Opc = N->getOpcode() == ISD::ADDCARRY ? AMDGPU::V_ADDC_U32_e64
                                                    : AMDGPU::V_SUBB_U32_e64;
     CurDAG->SelectNodeTo(
         N, Opc, N->getVTList(),
         {LHS, RHS, CI,
          CurDAG->getTargetConstant(0, {}, MVT::i1) /*clamp bit*/});
   } else {
+    // The result of a boolean operation is represented as a 32-bit/64-bit sgpr
+    // with bits set potentially even for inactive lanes, so mask them out here.
+    unsigned CondOpc = CI->getOpcode();
+    if (CondOpc == ISD::AND || CondOpc == ISD::OR || CondOpc == ISD::XOR) {
+      auto ST = static_cast<const GCNSubtarget *>(Subtarget);
+      CI = SDValue(
+          CurDAG->getMachineNode(
+              ST->isWave32() ? AMDGPU::S_AND_B32 : AMDGPU::S_AND_B64, DL,
+              MVT::i1,
+              CurDAG->getRegister(
+                  ST->isWave32() ? AMDGPU::EXEC_LO : AMDGPU::EXEC, MVT::i1),
+              CI),
+          0);
+    }
+
     unsigned Opc = N->getOpcode() == ISD::ADDCARRY ? AMDGPU::S_ADD_CO_PSEUDO
                                                    : AMDGPU::S_SUB_CO_PSEUDO;
     CurDAG->SelectNodeTo(N, Opc, N->getVTList(), {LHS, RHS, CI});
   }
 }
 
 void AMDGPUDAGToDAGISel::SelectUADDO_USUBO(SDNode *N) {
   // The name of the opcodes are misleading. v_add_i32/v_sub_i32 have unsigned

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

+; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck %s -check-prefixes=CHECK,GFX9
+; RUN: llc -march=amdgcn -mcpu=gfx1010 -verify-machineinstrs < %s | FileCheck %s -check-prefixes=CHECK,GFX10
+
+; Test that unused lanes are masked out in the s_xor result before it is used as condition code.
+
+; CHECK-LABEL: {{^}}combine_add_zext_xor:
+
+; GFX9: s_xor_b64 [[XOR:s\[[0-9]+:[0-9]+\]]]
+; GFX9: s_and_b64 [[CC:s\[[0-9]+:[0-9]+\]]], exec, [[XOR]]
+; GFX9: s_cmp_lg_u64 [[CC]], 0
+; GFX10: s_xor_b32 [[XOR:s[0-9]+]]
+; GFX10: s_and_b32 [[CC:s[0-9]+]], exec_lo, [[XOR]]
+; GFX10: s_cmpk_lg_u32 [[CC]], 0
+; CHECK: s_addc_u32
+
+define void @combine_add_zext_xor() {
+.entry:
+  br label %.exit
+
+.exit:                                            ; preds = %10, %.entry
+  %.0311 = phi i32 [ 1050, %.entry ], [ 0, %10 ]
+  %.0 = phi i32 [ 0, %.entry ], [ %11, %10 ]
+  %.2 = phi i32 [ 0, %.entry ], [ %12, %10 ]
+  %0 = call <4 x i32> @llvm.amdgcn.image.load.1d.v4i32.i32(i32 15, i32 %.0311, <8 x i32> undef, i32 0, i32 1)
+  %.i112 = extractelement <4 x i32> %0, i32 1
+  %.i3 = extractelement <4 x i32> %0, i32 3
+  br i1 undef, label %10, label %1
+
+1:                                                ; preds = %.exit
+  %2 = or i32 0, %.i112
+  %3 = or i32 0, %2
+  %4 = icmp eq i32 %3, 0
+  %5 = or i32 %.i3, %3
+  %6 = icmp eq i32 %5, 0
+  %7 = icmp eq i32 %.i3, 1
+  %8 = and i1 %7, %4
+  %9 = or i1 %6, %8
+  br label %10
+
+10:                                               ; preds = %1, %.exit
+  %.2.0.in.in = phi i1 [ %9, %1 ], [ undef, %.exit ]
+  %.2.0.in = xor i1 %.2.0.in.in, true
+  %.2.0 = zext i1 %.2.0.in to i32
+  %11 = add i32 %.0, %.2.0
+  %12 = add i32 %.2, %.2.0
+  %13 = icmp sgt i32 %.0311, -1050
+  br i1 %13, label %.exit, label %14
+
+14:                                               ; preds = %10
+  %15 = add i32 %.2, %.2.0
+  %16 = add i32 0, %.2.0
+  %17 = add i32 %.0, %.2.0
+  %18 = or i32 %17, %16
+  %19 = or i32 %15, %18
+  %20 = or i32 undef, %19
+  ret void
+}
+
+attributes #0 = { nounwind readonly willreturn }
+declare <4 x i32> @llvm.amdgcn.image.load.1d.v4i32.i32(i32 immarg, i32, <8 x i32>, i32 immarg, i32 immarg) #0