[GISelValueTracking] Add test case for G_PTRTOINT

[arichardson]

While we can only reason about the index/address, the G_PTRTOINT
operations returns all representation bits, so we can't assume the
remaining ones are all zeroes. This behaviour was clarified as part of
the discussion in https://discourse.llvm.org/t/clarifiying-the-semantics-of-ptrtoint/83987/54.
The LangRef semantics of ptrtoint being a full representation bitcast
were documented in #139349.

Prior to 77c8d21 we were incorrectly
assuming known zeroes beyond the index size even if the input was
completely unknown. This commit adds a test case for G_PTRTOINT which
was omitted from that change.

See #139598

[llvmbot]

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-amdgpu

Author: Alexander Richardson (arichardson)

Changes

While we can only reason about the index/address, the G_PTRTOINT
operations returns all representation bits, so we can't assume the
remaining ones are all zeroes.
This behaviour was clarified as part of the discussion in
https://discourse.llvm.org/t/clarifiying-the-semantics-of-ptrtoint/83987/54.
The LangRef semantics of ptrtoint being a full representation bitcast
were documented in #139349.

Fixes: #139598

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/GlobalISel/GISelValueTracking.cpp (+3-1)
• (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/ptrtoint-ptrtoaddr-p8.ll (+14-27)

diff --git a/llvm/lib/CodeGen/GlobalISel/GISelValueTracking.cpp b/llvm/lib/CodeGen/GlobalISel/GISelValueTracking.cpp
index 12fe28b29e5c8..b7e0a43f2fb64 100644
--- a/llvm/lib/CodeGen/GlobalISel/GISelValueTracking.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/GISelValueTracking.cpp
@@ -483,8 +483,10 @@ void GISelValueTracking::computeKnownBitsImpl(Register R, KnownBits &Known,
     if (Opcode == TargetOpcode::G_ASSERT_ZEXT)
       SrcBitWidth = MI.getOperand(2).getImm();
     else {
+      // For G_PTRTOINT all representation bits are returned even though only
+      // the address bits can be reasoned about generically.
       SrcBitWidth = SrcTy.isPointer()
-                        ? DL.getIndexSizeInBits(SrcTy.getAddressSpace())
+                        ? DL.getPointerSizeInBits(SrcTy.getAddressSpace())
                         : SrcTy.getSizeInBits();
     }
     assert(SrcBitWidth && "SrcBitWidth can't be zero");
diff --git a/llvm/test/CodeGen/AMDGPU/GlobalISel/ptrtoint-ptrtoaddr-p8.ll b/llvm/test/CodeGen/AMDGPU/GlobalISel/ptrtoint-ptrtoaddr-p8.ll
index 6722a55e8da92..d762d7728df36 100644
--- a/llvm/test/CodeGen/AMDGPU/GlobalISel/ptrtoint-ptrtoaddr-p8.ll
+++ b/llvm/test/CodeGen/AMDGPU/GlobalISel/ptrtoint-ptrtoaddr-p8.ll
@@ -79,9 +79,9 @@ define <2 x i64> @ptrtoaddr_vec(ptr addrspace(8) %ignored, <2 x ptr addrspace(8)
 ; GISEL:       ; %bb.0:
 ; GISEL-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GISEL-NEXT:    v_mov_b32_e32 v0, v4
-; GISEL-NEXT:    v_mov_b32_e32 v1, v5
+; GISEL-NEXT:    v_and_b32_e32 v1, 0xffff, v5
+; GISEL-NEXT:    v_and_b32_e32 v3, 0xffff, v9
 ; GISEL-NEXT:    v_mov_b32_e32 v2, v8
-; GISEL-NEXT:    v_mov_b32_e32 v3, v9
 ; GISEL-NEXT:    s_setpc_b64 s[30:31]
 ;
 ; SDAG-LABEL: ptrtoaddr_vec:
@@ -129,31 +129,18 @@ define i256 @ptrtoint_ext(ptr addrspace(8) %ignored, ptr addrspace(8) %ptr) {
 ;; FIXME: this is wrong for the GlobalISel case, we are removing the trunc:
 ;; https://github.com/llvm/llvm-project/issues/139598
 define i256 @ptrtoaddr_ext(ptr addrspace(8) %ignored, ptr addrspace(8) %ptr) {
-; GISEL-LABEL: ptrtoaddr_ext:
-; GISEL:       ; %bb.0:
-; GISEL-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
-; GISEL-NEXT:    v_mov_b32_e32 v0, v4
-; GISEL-NEXT:    v_mov_b32_e32 v1, v5
-; GISEL-NEXT:    v_mov_b32_e32 v2, v6
-; GISEL-NEXT:    v_mov_b32_e32 v3, v7
-; GISEL-NEXT:    v_mov_b32_e32 v4, 0
-; GISEL-NEXT:    v_mov_b32_e32 v5, 0
-; GISEL-NEXT:    v_mov_b32_e32 v6, 0
-; GISEL-NEXT:    v_mov_b32_e32 v7, 0
-; GISEL-NEXT:    s_setpc_b64 s[30:31]
-;
-; SDAG-LABEL: ptrtoaddr_ext:
-; SDAG:       ; %bb.0:
-; SDAG-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
-; SDAG-NEXT:    v_mov_b32_e32 v0, v4
-; SDAG-NEXT:    v_and_b32_e32 v1, 0xffff, v5
-; SDAG-NEXT:    v_mov_b32_e32 v2, 0
-; SDAG-NEXT:    v_mov_b32_e32 v3, 0
-; SDAG-NEXT:    v_mov_b32_e32 v4, 0
-; SDAG-NEXT:    v_mov_b32_e32 v5, 0
-; SDAG-NEXT:    v_mov_b32_e32 v6, 0
-; SDAG-NEXT:    v_mov_b32_e32 v7, 0
-; SDAG-NEXT:    s_setpc_b64 s[30:31]
+; CHECK-LABEL: ptrtoaddr_ext:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; CHECK-NEXT:    v_mov_b32_e32 v0, v4
+; CHECK-NEXT:    v_and_b32_e32 v1, 0xffff, v5
+; CHECK-NEXT:    v_mov_b32_e32 v2, 0
+; CHECK-NEXT:    v_mov_b32_e32 v3, 0
+; CHECK-NEXT:    v_mov_b32_e32 v4, 0
+; CHECK-NEXT:    v_mov_b32_e32 v5, 0
+; CHECK-NEXT:    v_mov_b32_e32 v6, 0
+; CHECK-NEXT:    v_mov_b32_e32 v7, 0
+; CHECK-NEXT:    s_setpc_b64 s[30:31]
   %ret = ptrtoaddr ptr addrspace(8) %ptr to i256
   ret i256 %ret
 }

[arichardson]

Now that we use the full bitwidth the high KnownBits are no longer zext'ed to zeroes. But maybe the better approahc would be to just do KnownBits on the address bits and set the high bits to unknown?
Otherwise we get the problem in #139598 where the trunc+ext is optimized away since we say the known upper bits are all zero even though it's an unknown input argument.

That should fix the issue as well?

[arichardson]

The current behaviour is definitely buggy since we lower G_PTRTOINT as a COPY node, but assume that it zeroes the upper bits for ptr addrspace(8). I think this patch is sufficient to prevent that issue.

[arichardson]

Thanks for the review, I'll update this to use a new baseline test that does not depend on the new ptrtoaddr.

[arichardson]

Looks like this was fixed while I was busy with other things - #140213 changed this code to drop the invalid width usage. I'll update this PR to just add a test for the ptrtoint case.