[DAGCombine] Fix always true condition in combineShiftToMULH

[jmmartinez]

Summary

I double checked and it seems the condition is already covered by the tests that are already present

[RKSimon]

Looking at D133768 it looks like you could add additional test case to dagcomb-mullohi.ll which has 2 'upper bit only' users (srl and sra)?

[jmmartinez]

In D139550#3978683, @RKSimon wrote:

Looking at D133768 it looks like you could add additional test case to dagcomb-mullohi.ll which has 2 'upper bit only' users (srl and sra)?

I'm not sure I follow. Isn't that case covered by the following function in the test case?

define i32 @mul_one_bit_hi_hi_u32(i32 %arg, i32 %arg1, i32* %arg2) {
  bb:
  %i = zext i32 %arg to i64
  %i3 = zext i32 %arg1 to i64
  %i4 = mul nsw i64 %i3, %i
  %i5 = lshr i64 %i4, 32
  %i6 = trunc i64 %i5 to i32
  store i32 %i6, i32* %arg2, align 4
  %i7 = lshr i64 %i4, 33
  %i8 = trunc i64 %i7 to i32
  ret i32 %i8
}

Or you mean having a srl and a sra in the same test-case?

define i32 @foo(i32 %arg, i32 %arg1, i32* %arg2) {
  bb:
  %i = zext i32 %arg to i64
  %i3 = zext i32 %arg1 to i64
  %i4 = mul nsw i64 %i3, %i
  %i5 = lshr i64 %i4, 32
  %i6 = trunc i64 %i5 to i32
  store i32 %i6, i32* %arg2, align 4
  %i7 = ashr i64 %i4, 33                  <-- change from logic to arithmetic
  %i8 = trunc i64 %i7 to i32
  ret i32 %i8
}

PS: Sorry for the delay, I was out-of-office.

[RKSimon]

In D139550#3988343, @jmmartinez wrote:

Or you mean having a srl and a sra in the same test-case?

Yes this is the test case I think we need to ensure we have a test case that has different behaviour for the if statement - if you precommit that srl+sra test then and rebase this patch the test codegen should change to match.

[jmmartinez]

In D139550#3988466, @RKSimon wrote:

In D139550#3988343, @jmmartinez wrote:

Or you mean having a srl and a sra in the same test-case?

Yes this is the test case I think we need to ensure we have a test case that has different behaviour for the if statement - if you precommit that srl+sra test then and rebase this patch the test codegen should change to match.

I thought the same, but I cannot seem to trigger the bug (my guess is that it's getting saved by some other pattern).

For this test case, the generated assembly is the same before and after the fix.

define i32 @mul_one_bit_hi_hi_u32_asr(i32 %arg, i32 %arg1, i32* %arg2) {
; CHECK-LABEL: mul_one_bit_hi_hi_u32_asr:
; CHECK:       ; %bb.0: ; %bb
; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
; CHECK-NEXT:    v_mul_hi_u32 v4, v1, v0
; CHECK-NEXT:    v_ashrrev_i64 v[0:1], 33, v[3:4]
; CHECK-NEXT:    flat_store_dword v[2:3], v4
; CHECK-NEXT:    s_waitcnt vmcnt(0) lgkmcnt(0)
; CHECK-NEXT:    s_setpc_b64 s[30:31]
bb:
  %i = zext i32 %arg to i64
  %i3 = zext i32 %arg1 to i64
  %i4 = mul nsw i64 %i3, %i
  %i5 = lshr i64 %i4, 32
  %i6 = trunc i64 %i5 to i32
  store i32 %i6, i32* %arg2, align 4
  %i7 = ashr i64 %i4, 33
  %i8 = trunc i64 %i7 to i32
  ret i32 %i8
}

[RKSimon]

OK - please can you add the test case to this patch so we at least have a canary if the other combine ever changes?

[jmmartinez]

Added test case

[RKSimon]

LGTM with a couple of minors - cheers!

llvm/test/CodeGen/AMDGPU/dagcomb-mullohi.ll

149

mul_one_bit_hi_hi_u32_lshr_ashr ?

169

add newline

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

static SDValue combineShiftToMULH(SDNode *N, SelectionDAG &DAG,
   if (!IsSignExt && !IsZeroExt)
     return SDValue();
 
   EVT NarrowVT = LeftOp.getOperand(0).getValueType();
   unsigned NarrowVTSize = NarrowVT.getScalarSizeInBits();
 
   // return true if U may use the lower bits of its operands
   auto UserOfLowerBits = [NarrowVTSize](SDNode *U) {
-    if (U->getOpcode() != ISD::SRL || U->getOpcode() != ISD::SRA) {
+    if (U->getOpcode() != ISD::SRL && U->getOpcode() != ISD::SRA) {
       return true;
     }
     ConstantSDNode *UShiftAmtSrc = isConstOrConstSplat(U->getOperand(1));
     if (!UShiftAmtSrc) {
       return true;
     }
     unsigned UShiftAmt = UShiftAmtSrc->getZExtValue();
     return UShiftAmt < NarrowVTSize;

llvm/test/CodeGen/AMDGPU/dagcomb-mullohi.ll

bb:
   %i5 = lshr i64 %i4, 32
   %i6 = trunc i64 %i5 to i32
   store i32 %i6, i32* %arg2, align 4
   %i7 = lshr i64 %i4, 31
   %i8 = trunc i64 %i7 to i32
   ret i32 %i8
 }
 
-define i32 @mul_one_bit_hi_hi_u32(i32 %arg, i32 %arg1, i32* %arg2) {
-; CHECK-LABEL: mul_one_bit_hi_hi_u32:
+define i32 @mul_one_bit_hi_hi_u32_lshr_lshr(i32 %arg, i32 %arg1, i32* %arg2) {
+; CHECK-LABEL: mul_one_bit_hi_hi_u32_lshr_lshr:
 ; CHECK:       ; %bb.0: ; %bb
 ; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; CHECK-NEXT:    v_mul_hi_u32 v0, v1, v0
 ; CHECK-NEXT:    flat_store_dword v[2:3], v0
 ; CHECK-NEXT:    v_lshrrev_b32_e32 v0, 1, v0
 ; CHECK-NEXT:    s_waitcnt vmcnt(0) lgkmcnt(0)
 ; CHECK-NEXT:    s_setpc_b64 s[30:31]
 bb:
   %i = zext i32 %arg to i64
   %i3 = zext i32 %arg1 to i64
   %i4 = mul nsw i64 %i3, %i
   %i5 = lshr i64 %i4, 32
   %i6 = trunc i64 %i5 to i32
   store i32 %i6, i32* %arg2, align 4
   %i7 = lshr i64 %i4, 33
   %i8 = trunc i64 %i7 to i32
   ret i32 %i8
 }
+
+define i32 @mul_one_bit_hi_hi_u32_lshr_ashr(i32 %arg, i32 %arg1, i32* %arg2) {
+; CHECK-LABEL: mul_one_bit_hi_hi_u32_lshr_ashr:
+; CHECK:       ; %bb.0: ; %bb
+; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; CHECK-NEXT:    v_mul_hi_u32 v4, v1, v0
+; CHECK-NEXT:    v_ashrrev_i64 v[0:1], 33, v[3:4]
+; CHECK-NEXT:    flat_store_dword v[2:3], v4
+; CHECK-NEXT:    s_waitcnt vmcnt(0) lgkmcnt(0)
+; CHECK-NEXT:    s_setpc_b64 s[30:31]
+bb:
+  %i = zext i32 %arg to i64
+  %i3 = zext i32 %arg1 to i64
+  %i4 = mul nsw i64 %i3, %i
+  %i5 = lshr i64 %i4, 32
+  %i6 = trunc i64 %i5 to i32
+  store i32 %i6, i32* %arg2, align 4
+  %i7 = ashr i64 %i4, 33
+  %i8 = trunc i64 %i7 to i32
+  ret i32 %i8
+}