[InferAddressSpaces] apply InferAddressSpaces to inttoptr-zext-ptrtoint address expression.

[yujc9]

Infer address spaces for inttoptr - zext - ptrtoint pattern.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-llvm-transforms

Author: None (yujc9)

Changes

Infer address spaces for inttoptr - zext - ptrtoint pattern.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp (+52-7)
• (added) llvm/test/Transforms/InferAddressSpaces/AMDGPU/zext-ptrint-conversion.ll (+58)

diff --git a/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp b/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
index 1bf50d79e5331e9..2b1229a11539585 100644
--- a/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
+++ b/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
@@ -300,6 +300,31 @@ static bool isNoopPtrIntCastPair(const Operator *I2P, const DataLayout &DL,
          (P2IOp0AS == I2PAS || TTI->isNoopAddrSpaceCast(P2IOp0AS, I2PAS));
 }
 
+// Check whether that's pointer bitcast using `ptrtoint`-`zext`-`inttoptr`
+static bool isZExtPtrIntCastPair(const Operator *I2P, const DataLayout &DL) {
+  assert(I2P->getOpcode() == Instruction::IntToPtr);
+  auto *ZExt = dyn_cast<Operator>(I2P->getOperand(0));
+  if (!ZExt || ZExt->getOpcode() != Instruction::ZExt)
+    return false;
+  auto *P2I = dyn_cast<Operator>(ZExt->getOperand(0));
+  if (!P2I || P2I->getOpcode() != Instruction::PtrToInt)
+    return false;
+  unsigned P2IOp0AS = P2I->getOperand(0)->getType()->getPointerAddressSpace();
+  unsigned I2PAS = I2P->getType()->getPointerAddressSpace();
+  unsigned P2IOp0SizeInBits = DL.getIntPtrType(P2I->getOperand(0)->getType())->getScalarSizeInBits();
+  unsigned I2PSizeInBits = DL.getIntPtrType(I2P->getType())->getScalarSizeInBits();
+  // Check:
+  // 1. `inttoptr` and `ptrtoint` are no-op casts
+  // 2. src address pointer and dst address pointer should be different address space and different size
+  return CastInst::isNoopCast(Instruction::CastOps(I2P->getOpcode()),
+                              I2P->getOperand(0)->getType(), I2P->getType(),
+                              DL) &&
+         CastInst::isNoopCast(Instruction::CastOps(P2I->getOpcode()),
+                              P2I->getOperand(0)->getType(), P2I->getType(),
+                              DL) &&
+         (P2IOp0AS != I2PAS) && (P2IOp0SizeInBits < I2PSizeInBits);
+}
+
 // Returns true if V is an address expression.
 // TODO: Currently, we consider only phi, bitcast, addrspacecast, and
 // getelementptr operators.
@@ -324,7 +349,8 @@ static bool isAddressExpression(const Value &V, const DataLayout &DL,
     return II && II->getIntrinsicID() == Intrinsic::ptrmask;
   }
   case Instruction::IntToPtr:
-    return isNoopPtrIntCastPair(Op, DL, TTI);
+    return isNoopPtrIntCastPair(Op, DL, TTI) ||
+           isZExtPtrIntCastPair(Op, DL);
   default:
     // That value is an address expression if it has an assumed address space.
     return TTI->getAssumedAddrSpace(&V) != UninitializedAddressSpace;
@@ -356,9 +382,15 @@ getPointerOperands(const Value &V, const DataLayout &DL,
     return {II.getArgOperand(0)};
   }
   case Instruction::IntToPtr: {
-    assert(isNoopPtrIntCastPair(&Op, DL, TTI));
-    auto *P2I = cast<Operator>(Op.getOperand(0));
-    return {P2I->getOperand(0)};
+    assert(isNoopPtrIntCastPair(&Op, DL, TTI) || isZExtPtrIntCastPair(&Op, DL));
+    if (isNoopPtrIntCastPair(&Op, DL, TTI)) {
+      auto *P2I = cast<Operator>(Op.getOperand(0));
+      return {P2I->getOperand(0)};
+    } else {
+      auto *ZExt = cast<Operator>(Op.getOperand(0));
+      auto *P2I = dyn_cast<Operator>(ZExt->getOperand(0));
+      return {P2I->getOperand(0)};
+    }
   }
   default:
     llvm_unreachable("Unexpected instruction type.");
@@ -521,8 +553,13 @@ InferAddressSpacesImpl::collectFlatAddressExpressions(Function &F) const {
     } else if (auto *ASC = dyn_cast<AddrSpaceCastInst>(&I)) {
       PushPtrOperand(ASC->getPointerOperand());
     } else if (auto *I2P = dyn_cast<IntToPtrInst>(&I)) {
-      if (isNoopPtrIntCastPair(cast<Operator>(I2P), *DL, TTI))
+      if (isNoopPtrIntCastPair(cast<Operator>(I2P), *DL, TTI)) {
         PushPtrOperand(cast<Operator>(I2P->getOperand(0))->getOperand(0));
+      } else if (isZExtPtrIntCastPair(cast<Operator>(I2P), *DL)) {
+        auto *ZExt = I2P->getOperand(0);
+        auto *P2I = cast<Operator>(ZExt)->getOperand(0);
+        PushPtrOperand(cast<Operator>(P2I)->getOperand(0));
+      }
     } else if (auto *RI = dyn_cast<ReturnInst>(&I)) {
       if (auto *RV = RI->getReturnValue();
           RV && RV->getType()->isPtrOrPtrVectorTy())
@@ -683,8 +720,16 @@ Value *InferAddressSpacesImpl::cloneInstructionWithNewAddressSpace(
     return SelectInst::Create(I->getOperand(0), NewPointerOperands[1],
                               NewPointerOperands[2], "", nullptr, I);
   case Instruction::IntToPtr: {
-    assert(isNoopPtrIntCastPair(cast<Operator>(I), *DL, TTI));
-    Value *Src = cast<Operator>(I->getOperand(0))->getOperand(0);
+    assert(isNoopPtrIntCastPair(cast<Operator>(I), *DL, TTI) ||
+           isZExtPtrIntCastPair(cast<Operator>(I), *DL));
+    Value *Src;
+    if (isNoopPtrIntCastPair(cast<Operator>(I), *DL, TTI)) {
+      Src = cast<Operator>(I->getOperand(0))->getOperand(0);
+    } else { // isZExtPtrIntCastPair
+      auto *ZExt = I->getOperand(0);
+      auto *P2I = cast<Operator>(ZExt)->getOperand(0);
+      Src = cast<Operator>(P2I)->getOperand(0);
+    }
     if (Src->getType() == NewPtrType)
       return Src;
 
diff --git a/llvm/test/Transforms/InferAddressSpaces/AMDGPU/zext-ptrint-conversion.ll b/llvm/test/Transforms/InferAddressSpaces/AMDGPU/zext-ptrint-conversion.ll
new file mode 100644
index 000000000000000..28acbb6a37a600b
--- /dev/null
+++ b/llvm/test/Transforms/InferAddressSpaces/AMDGPU/zext-ptrint-conversion.ll
@@ -0,0 +1,58 @@
+; RUN: opt -mtriple=amdgcn-amd-amdhsa -S -o - -passes=infer-address-spaces %s | FileCheck -check-prefixes=COMMON,AMDGCN %s
+; RUN: opt -S -o - -passes=infer-address-spaces -assume-default-is-flat-addrspace %s | FileCheck -check-prefixes=COMMON,NOTTI %s
+
+target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-ni:7:8"
+
+; COMMON-LABEL: @zext_ptrint_conversion(
+; AMDGCN-NEXT: store i32 0, ptr addrspace(3) %{{.*}}
+; AMDGCN-NEXT: ret void
+; NOTTI-NEXT: ptrtoint ptr addrspace(3) %{{.*}} to i64
+; NOTTI-NEXT: zext i32 %{{.*}} to i64
+; NOTTI-NEXT: inttoptr i64 %{{.*}} to ptr
+; NOTTI-NEXT: store i32 0, ptr %{{.*}}
+; NOTTI-NEXT: ret void
+define void @zext_ptrint_conversion(ptr addrspace(3) %x) {
+  %1 = ptrtoint ptr addrspace(3) %x to i32
+  %2 = zext i32 %1 to i64
+  %3 = inttoptr i64 %2 to ptr
+  store i32 0, ptr %3
+  ret void
+}
+
+; COMMON-LABEL: @non_zext_ptrint_conversion(
+; AMDGCN-NEXT: ptrtoint ptr addrspace(3) %{{.*}} to i16
+; AMDGCN-NEXT: zext i16 %{{.*}} to i64
+; AMDGCN-NEXT: inttoptr i64 %{{.*}} to ptr
+; AMDGCN-NEXT: store i32 0, ptr %{{.*}}
+; AMDGCN-NEXT: ret void
+; NOTTI-NEXT: ptrtoint ptr addrspace(3) %{{.*}} to i16
+; NOTTI-NEXT: zext i16 %{{.*}} to i64
+; NOTTI-NEXT: inttoptr i64 %{{.*}} to ptr
+; NOTTI-NEXT: store i32 0, ptr %{{.*}}
+; NOTTI-NEXT: ret void
+define void @non_zext_ptrint_conversion(ptr addrspace(3) %x) {
+  %1 = ptrtoint ptr addrspace(3) %x to i16
+  %2 = zext i16 %1 to i64
+  %3 = inttoptr i64 %2 to ptr
+  store i32 0, ptr %3
+  ret void
+}
+
+; COMMON-LABEL: @non_zext_ptrint_conversion2(
+; AMDGCN-NEXT: ptrtoint ptr addrspace(3) %{{.*}} to i32
+; AMDGCN-NEXT: zext i32 %{{.*}} to i128
+; AMDGCN-NEXT: inttoptr i128 %{{.*}} to ptr
+; AMDGCN-NEXT: store i32 0, ptr %{{.*}}
+; AMDGCN-NEXT: ret void
+; NOTTI-NEXT: ptrtoint ptr addrspace(3) %{{.*}} to i32
+; NOTTI-NEXT: zext i32 %{{.*}} to i128
+; NOTTI-NEXT: inttoptr i128 %{{.*}} to ptr
+; NOTTI-NEXT: store i32 0, ptr %{{.*}}
+; NOTTI-NEXT: ret void
+define void @non_zext_ptrint_conversion2(ptr addrspace(3) %x) {
+  %1 = ptrtoint ptr addrspace(3) %x to i32
+  %2 = zext i32 %1 to i128
+  %3 = inttoptr i128 %2 to ptr
+  store i32 0, ptr %3
+  ret void
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[arsenm]

I'm not sure this is obviously safe

[yujc9]

The reason for implementing this patch is to eliminate the cast for the pattern inttoptr - zext - ptrtoint.

define void @zext_ptrint_conversion(ptr addrspace(3) %x) {
  %tmp1 = ptrtoint ptr addrspace(3) %x to i32
  %tmp2 = zext i32 %tmp1 to i64
  %tmp3 = inttoptr i64 %tmp2 to ptr
  store i32 0, ptr %tmp3
  ret void
}

For instance, if we try to store a value to a generic pointer %tmp3 which is converted from a pointer %x with address space 3. If these two pointers have different size, an zext operation may be inserted. Since zext does not introduce any information, storing a value to the generic pointer %tmp3 should be equivalent to storing to ptr addrspace(3) %x directly.
To keep the transformation safe, the ptrtoint and inttoptr operation should be no-op cast. In addition, we require these two pointers in different address spaces and different size so that no extra information is added by zext operation.
Not sure other possibilities to make this conversion unsafe.

[yujc9]

cmd.exe /C "cd . && C:\BuildTools\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe -E vs_link_exe --intdir=tools\flang\unittests\Decimal\CMakeFiles\quick-sanity-test.test.dir --rc="C:\Program Files (x86)\Windows Kits\10\bin\10.0.19041.0\x64\rc.exe" --mt="C:\Program Files (x86)\Windows Kits\10\bin\10.0.19041.0\x64\mt.exe" --manifests -- C:\BuildTools\VC\Tools\MSVC\14.29.30133\bin\Hostx64\x64\link.exe /nologo tools\flang\unittests\Decimal\CMakeFiles\quick-sanity-test.test.dir\quick-sanity-test.cpp.obj /out:tools\flang\unittests\Decimal\quick-sanity-test.test.exe /implib:lib\quick-sanity-test.test.lib /pdb:tools\flang\unittests\Decimal\quick-sanity-test.test.pdb /version:0.0 /machine:x64 /STACK:10000000 /INCREMENTAL:NO /subsystem:console lib\LLVMSupport.lib lib\FortranDecimal.lib psapi.lib shell32.lib ole32.lib uuid.lib advapi32.lib ws2_32.lib delayimp.lib -delayload:shell32.dll -delayload:ole32.dll lib\LLVMDemangle.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && cd ." --

mt.exe
: general error c101008d: Failed to write the updated manifest to the
resource of file
"tools\flang\unittests\Decimal\quick-sanity-test.test.exe". Operation
did not complete successfully because the file contains a virus or
potentially unwanted software.

---

I got this CI failure message when building on Windows in buildkite . It is unlikely caused by this patch. Do you have any clues?

[arsenm]

I got this CI failure message when building on Windows in buildkite . It is unlikely caused by this patch. Do you have any clues?

Common spurious buildbot error

[arichardson]

A few comments but otherwise it looks sensible to me. I'll defer to folks more familiar with InferAddressSpace to give the merge approval.

[yujc9]

I was wondering what I need to do to get it merged? Or do I need change something?

[arsenm]

I was wondering what I need to do to get it merged? Or do I need change something?

I'm not convinced this is safe. Why do you need this pattern handled? inttoptr/ptrtoint destroys pointer provenance and the general guideline is to never touch them. We only special case the case we do now because the IR is missing a way to bitcast between pointers with the same size. Is there some rationale this is safe wrt provenance?

[arsenm]

Not no-op cast doesn't mean zero ext. This code is nonsense for AMDGPU for example

[yujc9]

I actually don't introduce another no-op cast, but another special case. This code works for AMDGPU in fact. In AMDGPU, the pointer with address space 3 is 32 bit, while the pointer with address space 0 is 64 bit. So, no-op cast won't handle this conversion. That's why there is a zero ext.
Introducing this pattern does similar thing We only special case the case we do now because the IR is missing a way to bitcast between pointers with the same size, but "missing a way to convert between pointers with different size".