AMDGPU: Only allow 31bit positive offset for scratch

[ruiling]

This just helps code safer as how the value interpreted by hardware might change.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Ruiling, Song (ruiling)

Changes

This just helps code safer as how the value interpreted by hardware might change.

---

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

1 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp (+1-1)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp b/llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
index cd810f0b43e50db..014026dd209da25 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
@@ -1859,7 +1859,7 @@ bool AMDGPUDAGToDAGISel::SelectScratchSVAddr(SDNode *N, SDValue Addr,
       std::tie(SplitImmOffset, RemainderOffset)
         = TII->splitFlatOffset(COffsetVal, AMDGPUAS::PRIVATE_ADDRESS, true);
 
-      if (isUInt<32>(RemainderOffset)) {
+      if (RemainderOffset > 0 && isInt<32>(RemainderOffset)) {
         SDNode *VMov = CurDAG->getMachineNode(
           AMDGPU::V_MOV_B32_e32, SL, MVT::i32,
           CurDAG->getTargetConstant(RemainderOffset, SDLoc(), MVT::i32));

[arsenm]

Also needs matching GlobalISel change? Is it testable?

[ruiling]

The GlobalISel does not have such logic, so no changes needed there. I am not quite sure do we really need a test for this. I guess this can only be tested with out-of-bounds access, as we cannot allocate too much scratch, right? I felt the change is not quite useful in practice. what do you think?

[arsenm]

I think it's good to get all the edge cases right, even if it's an unrealistically large allocation. We haven't sorted out how TLS should work, but it's possible we'll end up using scratch instructions to access it at weird offsets

[ruiling]

While we can write LLVM IR like: %ptr = getelementptr inbounds i8, ptr addrspace(5) %arg, i64 u0x80000000 for a positive offset with 31bit being one, it will be lowered into t4: i32 = add t2, Constant:i32<-2147483648> as our private address is 32bit. Seems like we cannot make a test to show difference here.

[ruiling]

I cannot think of ways to show the existing code is wrong. So it should be correct.