[PATCH] D150002: [AMDGPU] Fix crash with 160-bit p7's by manually defining getPointerTy

Fri May 5 15:14:40 PDT 2023

krzysz00 created this revision.
Herald added subscribers: kosarev, foad, kerbowa, hiraditya, arichardson, tpr, dstuttard, yaxunl, jvesely, kzhuravl, arsenm.
Herald added a project: All.
krzysz00 requested review of this revision.
Herald added subscribers: llvm-commits, wdng.
Herald added a project: LLVM.

While pointers in address space 7 (128 bit rsrc + 32 bit offset)
should be rewritten out of the code before IR translation on AMDGPU,
higher-level analyses may still call MVT getPointerTy() and the like
on the target machine. Currently, since there is no MVT::i160, this
operation ends up causing crashes.

The changes to the data layout that caused such crashes were D149776 <https://reviews.llvm.org/D149776>.

This patch, which simply causes getPointerTy() to return the
inaccurate, but close enough, MVT::i128 for address space 7 on AMDGPU,
is the fastest fix but not the best. Potential alternative solutions
include adjusting getPointerTy() to return an EVT or adding MVT::i160
and MVT::i256, both of which are rather disruptive to the rest of the
compiler.

(Note that with this change, trying to send a function that takes or
returns ptr addrspace(7) through IR translation still crashes, though
in a different way.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150002

Files:
  llvm/lib/Target/AMDGPU/SIISelLowering.cpp
  llvm/lib/Target/AMDGPU/SIISelLowering.h
  llvm/test/Transforms/IndVarSimplify/AMDGPU/addrspace-7-doesnt-crash.ll


Index: llvm/test/Transforms/IndVarSimplify/AMDGPU/addrspace-7-doesnt-crash.ll
===================================================================

--- /dev/null
+++ llvm/test/Transforms/IndVarSimplify/AMDGPU/addrspace-7-doesnt-crash.ll
@@ -0,0 +1,31 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
+; RUN: opt -passes=indvars -S < %s | FileCheck %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-G1-ni:7:8"
+target triple = "amdgcn--amdpal"
+
+define void @f(ptr addrspace(7) %arg) {
+; CHECK-LABEL: define void @f
+; CHECK-SAME: (ptr addrspace(7) [[ARG:%.*]]) {
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    br label [[BB1:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    br i1 false, label [[BB2:%.*]], label [[BB1]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[SCEVGEP:%.*]] = getelementptr i8, ptr addrspace(7) [[ARG]], i32 8
+; CHECK-NEXT:    br label [[BB3:%.*]]
+; CHECK:       bb3:
+; CHECK-NEXT:    [[I4:%.*]] = load i32, ptr addrspace(7) [[SCEVGEP]], align 4
+; CHECK-NEXT:    br label [[BB3]]
+;
+bb:
+  br label %bb1
+bb1:
+  %i = getelementptr i32, ptr addrspace(7) %arg, i32 2
+  br i1 false, label %bb2, label %bb1
+bb2:
+  br label %bb3
+bb3:
+  %i4 = load i32, ptr addrspace(7) %i, align 4
+  br label %bb3
+}
Index: llvm/lib/Target/AMDGPU/SIISelLowering.h
===================================================================
--- llvm/lib/Target/AMDGPU/SIISelLowering.h
+++ llvm/lib/Target/AMDGPU/SIISelLowering.h
@@ -273,6 +273,12 @@
 
   bool isShuffleMaskLegal(ArrayRef<int> /*Mask*/, EVT /*VT*/) const override;
 
+  // While address space 7 should never make it to codegen, it still needs to
+  // have a MVT to prevent some analyses that query this function from breaking,
+  // so lie and say that they fit into 128 bits.
+  MVT getPointerTy(const DataLayout &DL, unsigned AS) const override;
+  MVT getPointerMemTy(const DataLayout &DL, unsigned AS) const override;
+
   bool getTgtMemIntrinsic(IntrinsicInfo &, const CallInst &,
                           MachineFunction &MF,
                           unsigned IntrinsicID) const override;
Index: llvm/lib/Target/AMDGPU/SIISelLowering.cpp
===================================================================
--- llvm/lib/Target/AMDGPU/SIISelLowering.cpp
+++ llvm/lib/Target/AMDGPU/SIISelLowering.cpp
@@ -979,6 +979,24 @@
   return memVTFromLoadIntrData(ST->getContainedType(0), MaxNumLanes);
 }
 
+/// Map address space 7 to MVT::i128 because that's its in-memory
+/// representation. This doesn't ultimately matter because address space 7
+/// pointers should all be eliminated by codegen time, but there are analyses
+/// that, ultimately, depend on all pointers mapping to a MachineValueType. This
+/// is a lie, since the pointers are bigger than that, but it'll give cost
+/// models reasonable numbers to work with without being too out-there.
+MVT SITargetLowering::getPointerTy(const DataLayout &DL, unsigned AS) const {
+  if (AMDGPUAS::BUFFER_FAT_POINTER == AS && DL.getPointerSizeInBits(AS) > 128)
+    return MVT::i128;
+  return AMDGPUTargetLowering::getPointerTy(DL, AS);
+}
+
+MVT SITargetLowering::getPointerMemTy(const DataLayout &DL, unsigned AS) const {
+  if (AMDGPUAS::BUFFER_FAT_POINTER == AS && DL.getPointerSizeInBits(AS) > 128)
+    return MVT::i128;
+  return AMDGPUTargetLowering::getPointerMemTy(DL, AS);
+}
+
 bool SITargetLowering::getTgtMemIntrinsic(IntrinsicInfo &Info,
                                           const CallInst &CI,
                                           MachineFunction &MF,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D150002.519989.patch
Type: text/x-patch
Size: 3716 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230505/2c728281/attachment.bin>
