[PATCH] D81938: [InferAddressSpaces] Handle the pair of `ptrtoint`/`inttoptr`.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu:66
+// COMMON-LABEL: define amdgpu_kernel void @_Z7kernel41S(%struct.S.coerce %s.coerce)
+// OPT: %0 = extractvalue %struct.S.coerce %s.coerce, 0
+// OPT: %1 = extractvalue %struct.S.coerce %s.coerce, 1
----------------
Probably should use filecheck variables to avoid depending on the numbers


================
Comment at: clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu:69
+// OPT: %2 = load i32, i32 addrspace(1)* %0, align 4
+// OPT: %inc = add nsw i32 %2, 1
+// OPT: store i32 %inc, i32 addrspace(1)* %0, align 4
----------------
I think relying on the name may fail in a release build, filecheck variable


================
Comment at: llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp:237-239
+         TTI->isNoopAddrSpaceCast(
+             P2I->getOperand(0)->getType()->getPointerAddressSpace(),
+             I2P->getType()->getPointerAddressSpace());
----------------
arsenm wrote:
> hliao wrote:
> > arsenm wrote:
> > > Can you also elaborate on why the isNoopAddrSpaceCast check is needed? I'm not 100% sure it is in all contexts, so want to be sure to document that
> > As we may convert a pair of ptr/int casts into an `addrspacecast` if both of them are no-op casts, if that's not a no-op `addrspacecast` under a specific target, we may introduce an invalid `addrspacecast` based on the current IR spec.
> > 
> I mean there needs to be comment explaining all of this
I think this is missing one of the key points I'm worried about for the IR semantics.

I believe if you were to invalidly reinterpret a pointer with another address space, it would be undefined to access the invalid pointer and thus you can use this to infer. In pure arithmetic cases, I'm not sure this would hold. This check is to avoid breaking cases where it may still be valid to do something with reinterpreted pointer bits other than access memory


================
Comment at: llvm/test/Transforms/InferAddressSpaces/noop-ptrint-pair.ll:2
+; RUN: opt -mtriple=amdgcn-amd-amdhsa -S -o - -infer-address-spaces %s | FileCheck -check-prefixes=COMMON,AMDGCN %s
+; RUN: opt -S -o - -infer-address-spaces %s | FileCheck -check-prefixes=COMMON,NOTTI %s
+
----------------
The pass early exits without TTI since there's no flat address space value, so this isn't really testing anything. We would have to add a flag for the flat value to bypass it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81938/new/

https://reviews.llvm.org/D81938