[GlobalISel] Check width of APInts in Reassoc PtrAdd combine

[mbrkusanin]

No description provided.

[llvmbot]

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-amdgpu

Author: Mirko Brkušanin (mbrkusanin)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/84335.diff

2 Files Affected:

• (modified) llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp (+2-1)
• (added) llvm/test/CodeGen/AMDGPU/GlobalISel/combine-ptradd-reassociation.mir (+49)

diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index 2f18a64ca285bd..d772f3d228d0de 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -4653,7 +4653,8 @@ bool CombinerHelper::matchReassocFoldConstantsInSubTree(GPtrAdd &MI,
     return false;
 
   MatchInfo = [=, &MI](MachineIRBuilder &B) {
-    auto NewCst = B.buildConstant(MRI.getType(Src2Reg), *C1 + *C2);
+    auto NewCst = B.buildConstant(MRI.getType(Src2Reg),
+                                  C1->sextOrTrunc(C2->getBitWidth()) + *C2);
     Observer.changingInstr(MI);
     MI.getOperand(1).setReg(LHSSrc1);
     MI.getOperand(2).setReg(NewCst.getReg(0));
diff --git a/llvm/test/CodeGen/AMDGPU/GlobalISel/combine-ptradd-reassociation.mir b/llvm/test/CodeGen/AMDGPU/GlobalISel/combine-ptradd-reassociation.mir
new file mode 100644
index 00000000000000..67c8548f9272a9
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/GlobalISel/combine-ptradd-reassociation.mir
@@ -0,0 +1,49 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=amdgcn -run-pass=amdgpu-prelegalizer-combiner -verify-machineinstrs %s -o - | FileCheck %s
+
+---
+name:            test_different_sizes_64_32
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $sgpr0_sgpr1
+
+    ; CHECK-LABEL: name: test_different_sizes_64_32
+    ; CHECK: liveins: $sgpr0_sgpr1
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s64) = COPY $sgpr0_sgpr1
+    ; CHECK-NEXT: [[INTTOPTR:%[0-9]+]]:_(p1) = G_INTTOPTR [[COPY]](s64)
+    ; CHECK-NEXT: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 12
+    ; CHECK-NEXT: [[PTR_ADD:%[0-9]+]]:_(p1) = G_PTR_ADD [[INTTOPTR]], [[C]](s32)
+    ; CHECK-NEXT: SI_RETURN implicit [[PTR_ADD]](p1)
+    %0:_(s64) = COPY $sgpr0_sgpr1
+    %1:_(s64) = G_CONSTANT i64 8
+    %2:_(s32) = G_CONSTANT i32 4
+    %3:_(p1) = G_INTTOPTR %0(s64)
+    %4:_(p1) = G_PTR_ADD %3, %1(s64)
+    %5:_(p1) = G_PTR_ADD %4, %2(s32)
+    SI_RETURN implicit %5(p1)
+...
+---
+name:            test_different_sizes_32_64
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $sgpr0_sgpr1
+
+    ; CHECK-LABEL: name: test_different_sizes_32_64
+    ; CHECK: liveins: $sgpr0_sgpr1
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s64) = COPY $sgpr0_sgpr1
+    ; CHECK-NEXT: [[INTTOPTR:%[0-9]+]]:_(p1) = G_INTTOPTR [[COPY]](s64)
+    ; CHECK-NEXT: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 12
+    ; CHECK-NEXT: [[PTR_ADD:%[0-9]+]]:_(p1) = G_PTR_ADD [[INTTOPTR]], [[C]](s64)
+    ; CHECK-NEXT: SI_RETURN implicit [[PTR_ADD]](p1)
+    %0:_(s64) = COPY $sgpr0_sgpr1
+    %1:_(s64) = G_CONSTANT i64 8
+    %2:_(s32) = G_CONSTANT i32 4
+    %3:_(p1) = G_INTTOPTR %0(s64)
+    %4:_(p1) = G_PTR_ADD %3, %2(s32)
+    %5:_(p1) = G_PTR_ADD %4, %1(s64)
+    SI_RETURN implicit %5(p1)
+...

[mbrkusanin]

Should we always pick larger one instead?

[tschuett]

unsigned BitWidth = std::max(C1->getBitWidth(), C2->getBitWidth());

see #82927

[jayfoad]

Allowing arbitrary widths for the RHS of G_PTR_ADD makes me very uncomfortable. I would much prefer enforcing that the RHS has the same bitwidth as the index size of the pointer's address space. See also #81473.

[tschuett]

If you ask for getIConstantVRegValWithLookThrough from a s64 register, there is no guarantee that the bitwidth of APInt is 64 bit. If I have some fancy non integral AS with 128bit pointers, why should the offset of G_PTR_ADD be 128bit?

The test case is using the pre-legalizer combiner. I bet the real assert was in a bigger input.

[jayfoad]

If you ask for getIConstantVRegValWithLookThrough from a s64 register, there is no guarantee that the bitwidth of APInt is 64 bit.

Yes that should be guaranteed. If getIConstantVRegValWithLookThrough looks through a truncation or extension, it truncates or extends the APInt accordingly before returning it. Do you have an example where this does not work?

[jayfoad]

Allowing arbitrary widths for the RHS of G_PTR_ADD makes me very uncomfortable. I would much prefer enforcing that the RHS has the same bitwidth as the index size of the pointer's address space. See also #81473.

#84352

[jayfoad]

If #84352 is accepted then this won't be necessary - you can rely on C1 and C2 both having the correct (same) width already.

[mbrkusanin]

Ok then