Revert "AMDGPU: Try to commute sub of boolean ext"

[tpr]

Summary

This reverts commit 69fcfb7d3597e0cdb5554b4e672e9032b411b167.

As shown in the test I attached to this commit, the change I reverted
causes a problem with "zext(cc1) - zext(cc2)". It commuted
the operands to the sub and used different logic to select the addc/subc
instruction:

sub zext (setcc), x => addcarry 0, x, setcc
sub sext (setcc), x => subcarry 0, x, setcc

... but that is bogus. I believe it is not possible to fold those commuted
patterns into any form of addcarry or subcarry. It may have worked as
intended before "AMDGPU: Change boolean content type to 0 or 1" because
the setcc was considered to be -1 rather than 1.

Change-Id: If2139421aa6c935cbd1d925af58fe4a4aa9e8f43

[tpr]

V2: Removed an existing test for the bogus fold.

[arsenm]

llvm/test/CodeGen/AMDGPU/combine-cond-add-sub.ll

141

I think leaving the broken tests with the new emitted code would be more helpful than just deleting them

[hakzsam]

FWIW, this fixes a regression that introduced important glitches with Dead Rising 4 with both RADV/AMDVLK. Can this be pushed?

[tpr]

Yes, sorry, I will do the test changes that Matt suggested so this can be approved and landed.

[tpr]

V3: Reinstated and fixed the removed test.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

if (VT != MVT::i32)
     return SDValue();
 
   SDLoc SL(N);
   SDValue LHS = N->getOperand(0);
   SDValue RHS = N->getOperand(1);
 
   // sub x, zext (setcc) => subcarry x, 0, setcc
   // sub x, sext (setcc) => addcarry x, 0, setcc
-
-  bool Commuted = false;
-  unsigned Opc = LHS.getOpcode();
-  if (Opc == ISD::ZERO_EXTEND || Opc == ISD::SIGN_EXTEND ||
-      Opc == ISD::ANY_EXTEND) {
-    std::swap(RHS, LHS);
-    Commuted = true;
-  }
-
-  Opc = RHS.getOpcode();
+  unsigned Opc = RHS.getOpcode();
   switch (Opc) {
   default: break;
   case ISD::ZERO_EXTEND:
   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 Zero = DAG.getConstant(0, SL, MVT::i32);
-    SDValue Args[3];
-    Args[2] = Cond;
-
-    if (Commuted) {
-      // sub zext (setcc), x => addcarry 0, x, setcc
-      // sub sext (setcc), x => subcarry 0, x, setcc
-      Args[0] = Zero;
-      Args[1] = LHS;
-      Opc = (Opc == ISD::SIGN_EXTEND) ? ISD::SUBCARRY : ISD::ADDCARRY;
-    } else {
-      Args[0] = LHS;
-      Args[1] = Zero;
-      Opc = (Opc == ISD::SIGN_EXTEND) ? ISD::ADDCARRY : ISD::SUBCARRY;
-    }
-
+    SDValue Args[] = { LHS, DAG.getConstant(0, SL, MVT::i32), Cond };
     return DAG.getNode(Opc, SL, VTList, Args);
   }
   }
 
   if (LHS.getOpcode() == ISD::SUBCARRY) {
     // sub (subcarry x, 0, cc), y => subcarry x, y, cc
     auto C = dyn_cast<ConstantSDNode>(LHS.getOperand(1));
     if (!C || !C->isNullValue())

llvm/test/CodeGen/AMDGPU/combine-cond-add-sub.ll

bb:
   %cmp = icmp ugt i32 %x, %y
   %ext = sext i1 %cmp to i32
   %adde = add i32 %v, %ext
   %sub = sub i32 %adde, %a
   store i32 %sub, i32 addrspace(1)* %gep, align 4
   ret void
 }
 
-; GCN-LABEL: {{^}}sub_sube_commuted:
-; GCN-DAG: v_cmp_gt_u32_e{{32|64}} [[CC:[^,]+]], v{{[0-9]+}}, v{{[0-9]+}}
-; GCN-DAG: buffer_load_dword [[V:v[0-9]+]],
-; GCN: v_addc_u32_e32 [[ADDC:v[0-9]+]], vcc, v{{[0-9]+}}, v{{[0-9]+}}, [[CC]]
-; GCN: v_add_i32_e32 {{.*}}, 0x64, [[ADDC]]
-
-; GFX9-LABEL: {{^}}sub_sube_commuted:
-; GFX9-DAG: v_cmp_gt_u32_e{{32|64}} [[CC:[^,]+]], v{{[0-9]+}}, v{{[0-9]+}}
-; GFX9-DAG: global_load_dword [[V:v[0-9]+]],
-; GFX9: v_addc_co_u32_e32 [[ADDC:v[0-9]+]], vcc, v{{[0-9]+}}, v{{[0-9]+}}, [[CC]]
-; GFX9: v_add_u32_e32 {{.*}}, 0x64, [[ADDC]]
-define amdgpu_kernel void @sub_sube_commuted(i32 addrspace(1)* nocapture %arg, i32 %a) {
-bb:
-  %x = tail call i32 @llvm.amdgcn.workitem.id.x()
-  %y = tail call i32 @llvm.amdgcn.workitem.id.y()
-  %gep = getelementptr inbounds i32, i32 addrspace(1)* %arg, i32 %x
-  %v = load i32, i32 addrspace(1)* %gep, align 4
-  %cmp = icmp ugt i32 %x, %y
-  %ext = sext i1 %cmp to i32
-  %adde = add i32 %v, %ext
-  %sub = sub i32 %adde, %a
-  %sub2 = sub i32 100, %sub
-  store i32 %sub2, i32 addrspace(1)* %gep, align 4
-  ret void
-}
-
 ; GCN-LABEL: {{^}}sube_sub:
 ; GCN: v_cmp_gt_u32_e{{32|64}} [[CC:[^,]+]], v{{[0-9]+}}, v{{[0-9]+}}
 ; GCN: v_subb_u32_e{{32|64}} v{{[0-9]+}}, {{[^,]+}}, v{{[0-9]+}}, v{{[0-9]+}}, [[CC]]
 ; GCN-NOT: v_cndmask
 ; GCN-NOT: v_sub
 
 ; GFX9-LABEL: {{^}}sube_sub:
 ; GFX9: v_subb_co_u32_e{{32|64}} v{{[0-9]+}}, vcc
bb:
   %cmp = icmp ugt i32 %x, %y
   %ext = zext i1 %cmp to i32
   %adde = sub i32 %v, %ext
   %add2 = sub i32 %adde, %a
   store i32 %add2, i32 addrspace(1)* %gep, align 4
   ret void
 }
 
-; Check case where sub is commuted with zext
-; GCN-LABEL: {{^}}sub_zext_setcc_commute:
-; GCN: v_cmp_gt_u32_e32 vcc, v
-; GCN-NOT: vcc
-; GCN: v_addc_u32_e32 [[ADDC:v[0-9]+]], vcc, v{{[0-9]+}}, v{{[0-9]+}}, vcc
-; GCN: v_subrev_i32_e32 [[RESULT:v[0-9]+]], vcc, s{{[0-9]+}}, [[ADDC]]
-define amdgpu_kernel void @sub_zext_setcc_commute(i32 addrspace(1)* nocapture %arg, i32 %a, i32%b) {
-bb:
-  %x = tail call i32 @llvm.amdgcn.workitem.id.x()
-  %y = tail call i32 @llvm.amdgcn.workitem.id.y()
-  %gep = getelementptr inbounds i32, i32 addrspace(1)* %arg, i32 %x
-  %v = load i32, i32 addrspace(1)* %gep, align 4
-  %cmp = icmp ugt i32 %x, %y
-  %ext = zext i1 %cmp to i32
-  %adde = sub i32 %v, %ext
-  %sub = sub i32 %a, %adde
-  %sub2 = sub i32 %sub, %b
-  store i32 %sub2, i32 addrspace(1)* %gep, align 4
-  ret void
-}
-
-; Check case where sub is commuted with sext
-; GCN-LABEL: {{^}}sub_sext_setcc_commute:
-; GCN: v_cmp_gt_u32_e32 vcc, v
-; GCN-NOT: vcc
-; GCN: v_subb_u32_e32 [[SUBB:v[0-9]+]], vcc, 0, v{{[0-9]+}}, vcc
-; GCN: v_add_i32_e32 [[ADD:v[0-9]+]], vcc, s{{[0-9]+}}, [[SUBB]]
-; GCN: v_subrev_i32_e32 [[RESULT:v[0-9]+]], vcc, s{{[0-9]+}}, [[ADD]]
-define amdgpu_kernel void @sub_sext_setcc_commute(i32 addrspace(1)* nocapture %arg, i32 %a, i32%b) {
-bb:
-  %x = tail call i32 @llvm.amdgcn.workitem.id.x()
-  %y = tail call i32 @llvm.amdgcn.workitem.id.y()
-  %gep = getelementptr inbounds i32, i32 addrspace(1)* %arg, i32 %x
-  %v = load i32, i32 addrspace(1)* %gep, align 4
-  %cmp = icmp ugt i32 %x, %y
-  %ext = sext i1 %cmp to i32
-  %adde = sub i32 %v, %ext
-  %sub = sub i32 %a, %adde
-  %sub2 = sub i32 %sub, %b
-  store i32 %sub2, i32 addrspace(1)* %gep, align 4
-  ret void
-}
-
 declare i1 @llvm.amdgcn.class.f32(float, i32) #0
 
 declare i32 @llvm.amdgcn.workitem.id.x() #0
 
 declare i32 @llvm.amdgcn.workitem.id.y() #0
 
 attributes #0 = { nounwind readnone speculatable }

llvm/test/CodeGen/AMDGPU/sub-zext-cc-zext-cc.ll

+; RUN: llc -mtriple=amdgcn--amdpal -mcpu=gfx900 -verify-machineinstrs <%s | FileCheck -check-prefixes=GCN %s
+;
+; This test checks that we have the correct fold for zext(cc1) - zext(cc2).
+;
+; GCN-LABEL: sub_zext_zext:
+; GCN: ds_read_b32 [[VAL:v[0-9]+]],
+; GCN-DAG: v_cmp_lt_f32{{.*}} [[CC1:s\[[0-9]+:[0-9]+\]]], 0, [[VAL]]
+; GCN-DAG: v_cmp_gt_f32{{.*}} vcc, 0, [[VAL]]
+; GCN: v_cndmask_{{.*}} [[ZEXTCC1:v[0-9]+]], 0, 1, [[CC1]]
+; GCN: v_subbrev{{.*}} {{v[0-9]+}}, vcc, 0, [[ZEXTCC1]], vcc
+;
+; Before the reversion that this test is attached to, the compiler commuted
+; the operands to the sub and used different logic to select the addc/subc
+; instruction:
+;    sub zext (setcc), x => addcarry 0, x, setcc
+;    sub sext (setcc), x => subcarry 0, x, setcc
+;
+; ... but that is bogus. I believe it is not possible to fold those commuted
+; patterns into any form of addcarry or subcarry.
+
+define amdgpu_cs float @sub_zext_zext() {
+.entry:
+
+  %t519 = load float, float addrspace(3)* null
+
+  %t524 = fcmp ogt float %t519, 0.000000e+00
+  %t525 = fcmp olt float %t519, 0.000000e+00
+  %t526 = zext i1 %t524 to i32
+  %t527 = zext i1 %t525 to i32
+  %t528 = sub nsw i32 %t526, %t527
+  %t529 = sitofp i32 %t528 to float
+  ret float %t529
+}
+