Fix for AMDGPU MUL_I24 known bits calculation

[ekuznetsov139]

Summary

At present, the code calculating known bits of AMDGPU MUL_I24 confuses the concepts of "non-negative number" and "positive number".

In some situations, it results in incorrect code. I have a case where the optimizer replaces the result of calculating MUL_I24(-5, 0) with -8.

[arsenm]

Needs testcase

[ekuznetsov139]

In D70367#1749187, @arsenm wrote:

Needs testcase

Do you want me to provide a testcase? and what exactly do you mean by "testcase" - to supply an example of the bug, or to write a unit test for llvm testing for the bug?

[ekuznetsov139]

As far as an example, compile the following

test.ll12 KBDownload

llc -mtriple amdgcn-amd-amdhsa -mcpu=gfx900 -mattr=-code-object-v3 -O2 -amdgpu-function-calls=0 -filetype=asm -o test.isa test.ll

The IR code:

%5 = tail call i32 @llvm.amdgcn.workitem.id.x() #28, !range !4
%tid_y = lshr i32 %5, 4
%tid_x = and i32 %5, 15

%y_div_5 = sdiv i32 %tid_y, 5
%6 = mul nsw i32 %y_div_5, -5
%y_mod_5 = add nsw i32 %6, %tid_y
%v1 = add nsw i32 %tid_x, %y_mod_5
%7 = icmp sgt i32 %v1, -2
%spec.select.i51 = select i1 %7, i32 %v1, i32 -2

The corresponding ISA:

v_and_b32_e32 v15, 15, v0
v_lshrrev_b32_e32 v14, 4, v0
s_load_dwordx2 s[0:1], s[4:5], 0x0
v_add3_u32 v0, v14, v15, -8
v_max_i32_e32 v0, -2, v0

[arsenm]

In D70367#1749255, @ekuznetsov139 wrote:

As far as an example, compile the following

test.ll12 KBDownload

llc -mtriple amdgcn-amd-amdhsa -mcpu=gfx900 -mattr=-code-object-v3 -O2 -amdgpu-function-calls=0 -filetype=asm -o test.isa test.ll

The IR code:

%5 = tail call i32 @llvm.amdgcn.workitem.id.x() #28, !range !4
%tid_y = lshr i32 %5, 4
%tid_x = and i32 %5, 15

%y_div_5 = sdiv i32 %tid_y, 5
%6 = mul nsw i32 %y_div_5, -5
%y_mod_5 = add nsw i32 %6, %tid_y
%v1 = add nsw i32 %tid_x, %y_mod_5
%7 = icmp sgt i32 %v1, -2
%spec.select.i51 = select i1 %7, i32 %v1, i32 -2

The corresponding ISA:

v_and_b32_e32 v15, 15, v0
v_lshrrev_b32_e32 v14, 4, v0
s_load_dwordx2 s[0:1], s[4:5], 0x0
v_add3_u32 v0, v14, v15, -8
v_max_i32_e32 v0, -2, v0

I mean this testcase needs to be prepared as a lit test for the test suite in the patch itself

[arsenm]

In D70367#1749263, @arsenm wrote:

In D70367#1749255, @ekuznetsov139 wrote:

As far as an example, compile the following

test.ll12 KBDownload

llc -mtriple amdgcn-amd-amdhsa -mcpu=gfx900 -mattr=-code-object-v3 -O2 -amdgpu-function-calls=0 -filetype=asm -o test.isa test.ll

The IR code:

%5 = tail call i32 @llvm.amdgcn.workitem.id.x() #28, !range !4
%tid_y = lshr i32 %5, 4
%tid_x = and i32 %5, 15

%y_div_5 = sdiv i32 %tid_y, 5
%6 = mul nsw i32 %y_div_5, -5
%y_mod_5 = add nsw i32 %6, %tid_y
%v1 = add nsw i32 %tid_x, %y_mod_5
%7 = icmp sgt i32 %v1, -2
%spec.select.i51 = select i1 %7, i32 %v1, i32 -2

The corresponding ISA:

v_and_b32_e32 v15, 15, v0
v_lshrrev_b32_e32 v14, 4, v0
s_load_dwordx2 s[0:1], s[4:5], 0x0
v_add3_u32 v0, v14, v15, -8
v_max_i32_e32 v0, -2, v0

I mean this testcase needs to be prepared as a lit test for the test suite in the patch itself

Should probably go in test/CodeGen/AMDGPU/mul_int24.ll and/or test/CodeGen/AMDGPU/mul_uint24.ll to go with similar tests

[arsenm]

llvm/test/CodeGen/AMDGPU/amdgpu-mul24-knownbits.ll

16–26

This testcase is far too big. You should be able to reduce it to a handful of instsructions, probably these ones

[foad]

The logic looks correct to me.

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

4455

Space between "if" and "(".

4457

Likewise.

[ekuznetsov139]

Simpler testcase

[arsenm]

llvm/test/CodeGen/AMDGPU/amdgpu-mul24-knownbits.ll

3

FUNC isn’t specified as a check prefix, but there’s also no reason to use a second prefix

4

Run line should be first. Most of the flags to llc can also be removed

5

Should use positive checks. I don’t know what this would exclude since we won’t emit anything with dashes

6

Drop this comment

18

Should probably merge this with the other mul24 test files

[ekuznetsov139]

llvm/test/CodeGen/AMDGPU/amdgpu-mul24-knownbits.ll

5

This would exclude

global_store_dword v[0:1], v2, off offset:-128

[arsenm]

llvm/test/CodeGen/AMDGPU/amdgpu-mul24-knownbits.ll

5

Ok, that’s not obvious. Positive checks are much less error prone

[ekuznetsov139]

llvm/test/CodeGen/AMDGPU/amdgpu-mul24-knownbits.ll

5

This is a negative test. We are testing to make sure that the optimizer does not assume %v1 to be always equal to -32. A negative check fits right in. A positive check would be harder to write since multiple possible correct codes could be generated.

[arsenm]

llvm/test/CodeGen/AMDGPU/amdgpu-mul24-knownbits.ll

5

That’s fine. A negative test is easily breakable and should be avoided

[arsenm]

llvm/test/CodeGen/AMDGPU/amdgpu-mul24-knownbits.ll

3

Just generate the checks at this point. This is going to successfully match the name of the function

[ekuznetsov139]

I give up. I don't know what kind of test you want. I'll let someone who knows how to do these things submit a patch.

[foad]

In D70367#1770168, @ekuznetsov139 wrote:

I give up. I don't know what kind of test you want. I'll let someone who knows how to do these things submit a patch.

Please don't give up! Matt's suggestion is to use exactly the .ll test case you already have, but to run utils/update_llc_test_checks.py on it to automatically generate the CHECK: (or GCN:) lines for it.

[arsenm]

In D70367#1770168, @ekuznetsov139 wrote:

I give up. I don't know what kind of test you want. I'll let someone who knows how to do these things submit a patch.

You can either use updatet_llc_test_checks, or follow the other examples in tesst/CodeGen/AMDGPU/mul_int24.ll

[ekuznetsov139]

I don't see how this is better than GCN-NOT:-128, but here you go

[arsenm]

LGTM

llvm/include/llvm/Support/KnownBits.h

   /// Returns true if this value is known to be non-negative.
   bool isNonNegative() const { return Zero.isSignBitSet(); }
 
+  /// Returns true if this value is known to be positive.
+  bool isStrictlyPositive() const { return Zero.isSignBitSet() && !One.isNullValue(); }
+
   /// Make this value negative.
   void makeNegative() {
     One.setSignBit();

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

     LHSKnown = LHSKnown.trunc(24);
     RHSKnown = RHSKnown.trunc(24);
 
-    bool Negative = false;
     if (Opc == AMDGPUISD::MUL_I24) {
       unsigned LHSValBits = 24 - LHSKnown.countMinSignBits();
       unsigned RHSValBits = 24 - RHSKnown.countMinSignBits();

       if (MaxValBits >= 32)
         break;
       bool LHSNegative = LHSKnown.isNegative();
-      bool LHSPositive = LHSKnown.isNonNegative();
+      bool LHSNonNegative = LHSKnown.isNonNegative();
+      bool LHSPositive = LHSKnown.isStrictlyPositive();
       bool RHSNegative = RHSKnown.isNegative();
-      bool RHSPositive = RHSKnown.isNonNegative();
-      if ((!LHSNegative && !LHSPositive) || (!RHSNegative && !RHSPositive))
-        break;
-      Negative = (LHSNegative && RHSPositive) || (LHSPositive && RHSNegative);
-      if (Negative)
-        Known.One.setHighBits(32 - MaxValBits);
-      else
+      bool RHSNonNegative = RHSKnown.isNonNegative();
+      bool RHSPositive = RHSKnown.isStrictlyPositive();
+
+      if ((LHSNonNegative && RHSNonNegative) || (LHSNegative && RHSNegative))
         Known.Zero.setHighBits(32 - MaxValBits);
+      else if ((LHSNegative && RHSPositive) || (LHSPositive && RHSNegative))
+        Known.One.setHighBits(32 - MaxValBits);
     } else {
       unsigned LHSValBits = 24 - LHSKnown.countMinLeadingZeros();
       unsigned RHSValBits = 24 - RHSKnown.countMinLeadingZeros();

llvm/test/CodeGen/AMDGPU/amdgpu-mul24-knownbits.ll

+
+; FUNC-LABEL: {{^}}test_mul24_knownbits_kernel:
+; RUN: llc -mtriple amdgcn-amd-amdhsa -mcpu=gfx900 -mattr=-code-object-v3 -O2 -amdgpu-function-calls=0 < %s | FileCheck --check-prefix=GCN %s
+; GCN-NOT: -128
+; Function Attrs: alwaysinline convergent norecurse nounwind
+define weak_odr amdgpu_kernel void @test_mul24_knownbits_kernel(float addrspace(1)* %p) #4 {
+entry:
+  %0 = tail call i32 @llvm.amdgcn.workitem.id.x() #28, !range !4
+  %tid = and i32 %0, 3
+  %1 = mul nsw i32 %tid, -5
+  %v1 = and i32 %1, -32
+  %v2 = sext i32 %v1 to i64 
+  %v3 = getelementptr inbounds float, float addrspace(1)* %p, i64 %v2
+  store float 0.000, float addrspace(1)* %v3, align 4
+  ret void
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare i32 @llvm.amdgcn.workitem.id.x() #20
+
+!4 = !{i32 0, i32 1024}

llvm/test/CodeGen/AMDGPU/mul_int24.ll

   ret void
 
 }
+
+
+; FUNC-LABEL: {{^}}test_mul24_knownbits_kernel:
+; RUN: llc -mtriple amdgcn-amd-amdhsa -mcpu=gfx900 -mattr=-code-object-v3 -O2 -amdgpu-function-calls=0 < %s | FileCheck --check-prefix=GCN %s
+; GCN-NOT: -128
+; Function Attrs: alwaysinline convergent norecurse nounwind
+define weak_odr amdgpu_kernel void @test_mul24_knownbits_kernel(float addrspace(1)* %p) #4 {
+entry:
+  %0 = addrspacecast float* %p to float addrspace(1)*
+  %1 = tail call i32 @llvm.amdgcn.workitem.id.x() #28, !range !4
+  %tid = and i32 %1, 3
+  %2 = mul nsw i32 %tid, -5
+  %v1 = and i32 %2, -32
+  %v2 = sext i32 %v1 to i64 
+  %v3 = getelementptr inbounds float, float addrspace(1)* %0, i64 %v2
+  store float 0.000, float addrspace(1)* %v3, align 4
+  ret void
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare i32 @llvm.amdgcn.workitem.id.x() #20
+
 attributes #0 = { nounwind }
+!4 = !{i32 0, i32 1024}