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

[krzysz00]

Summary

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.

This patch causes getPointerTy() to return the type MVT::v5i32
and getPointerMemTy() to be MVT::v8i32. These are accurate types,
but mean that we can't use vectors of address space 7 pointers during
codegen. This is mostly OK, since vectors of buffers aren't supported
in LPC anyway, but it's a noticable limitation.

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.

[arsenm]

Can you just use v5i32

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

26

Better to have a test with a non foldable branch

[krzysz00]

Move to MVT::v5i32, sacrificing the ability to handle vectors of p7 in codegen but being more correct otherwise.

[krzysz00]

The problem with using v5i32 as the MVT is that you get an inevitable crash when you getValueType() on a vector of p7's per https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/CodeGen/TargetLowering.h#L1520

Which ... having looked, could well be fine

[arsenm]

It also would not be difficult to just define i160. It should be easier than ever now that MVT is generated

[krzysz00]

@arsenm Given that, from what I can tell, MVT is the type for "things some architecture somewhere needed for codegen", I was somewhat concerned that adding MVT::i160 might break legalization or something.

But I would prefer to go that route if it's something you think everyone'll be OK with

[arsenm]

In D150002#4329912, @krzysz00 wrote:

@arsenm Given that, from what I can tell, MVT is the type for "things some architecture somewhere needed for codegen", I was somewhat concerned that adding MVT::i160 might break legalization or something.

I doubt it will break anything. Most everything looks for next-power-of-2 and wouldn't see it

[krzysz00]

I went looking and ran into, for example, TargetLoweringBase::computeRegisterProperties, which assumes the MVT integers all double in size.

[krzysz00]

We could probably special-case around it, but ... for temporarily un-breaking higher-level analyses that end up looking at TargetTransformInfo, we can probably either go for the vector type or lie.

... though, we could also hack in support for 2x5xi32 -> 10xi32 type transformations.

[krzysz00]

Ping to land this because there's active breakage? @foad @arsenm

[arichardson]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

982

Comment needs updating for the v5 change

[krzysz00]

Update comment fon the new approach

[dstuttard]

Confirmed that this fixes the issues we were observing.

[foad]

In D150002#4335104, @krzysz00 wrote:

Ping to land this because there's active breakage? @foad @arsenm

Yes please land this.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

990

Nit: why do you need the getPointerSizeInBits test?

[foad]

llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-non-integral-address-spaces-vectors.ll

4

Nit: you're not actually checking anything.

[krzysz00]

Update checks, coments, remase

[foad]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

990

I mean why do you need it at all? Don't we know statically that the size of BUFFER_FAT_POINTER is 160 bits?

[krzysz00]

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

990

Paranoia around making sure this isn't an old data layout or the like. I might even want to make the check more precise.

990

I was concerned that, for example, auto-upgrade might not fire or that you'd otherwise end up in a position where p7 doesn't have its usual definition and so thought to go ahead and check.

[arsenm]

In D150002#4330056, @krzysz00 wrote:

I went looking and ran into, for example, TargetLoweringBase::computeRegisterProperties, which assumes the MVT integers all double in size.

Just add it and see it anything breaks

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

990

Just drop it, there's no reason to try defending against that here

[dyung]

Just a heads up that we noticed in our internal testing that llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-non-integral-address-spaces-vectors.ll failed in a release build without assertions because it is looking for a crash that is caused by an assertion failure. I've added that requirement to the test in 97b73e3.

llvm/lib/Target/AMDGPU/SIISelLowering.h

public:
   bool isFPExtFoldable(const SelectionDAG &DAG, unsigned Opcode, EVT DestVT,
                        EVT SrcVT) const override;
 
   bool isFPExtFoldable(const MachineInstr &MI, unsigned Opcode, LLT DestTy,
                        LLT SrcTy) const override;
 
   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, to work around the lack of i160, map it to v5i32.
+  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;
 
   bool getAddrModeArguments(IntrinsicInst * /*I*/,
                             SmallVectorImpl<Value*> &/*Ops*/,
                             Type *&/*AccessTy*/) const override;
 

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

if (!ST)
     return memVTFromLoadIntrData(Ty, MaxNumLanes);
 
   // TFE intrinsics return an aggregate type.
   assert(ST->getNumContainedTypes() == 2 &&
          ST->getContainedType(1)->isIntegerTy(32));
   return memVTFromLoadIntrData(ST->getContainedType(0), MaxNumLanes);
 }
 
+/// Map address space 7 to MVT::v5i32 because that's its in-memory
+/// representation. This return value is vector-typed because there is no
+/// MVT::i160 and it is not clear if one can be added. While this could
+/// cause issues during codegen, these address space 7 pointers will be
+/// rewritten away by then. Therefore, we can return MVT::v5i32 in order
+/// to allow pre-codegen passes that query TargetTransformInfo, often for cost
+/// modeling, to work.
+MVT SITargetLowering::getPointerTy(const DataLayout &DL, unsigned AS) const {
+  if (AMDGPUAS::BUFFER_FAT_POINTER == AS && DL.getPointerSizeInBits(AS) == 160)
+    return MVT::v5i32;
+  return AMDGPUTargetLowering::getPointerTy(DL, AS);
+}
+/// Similarly, the in-memory representation of a p7 is {p8, i32}, aka
+/// v8i32 when padding is added.
+MVT SITargetLowering::getPointerMemTy(const DataLayout &DL, unsigned AS) const {
+  if (AMDGPUAS::BUFFER_FAT_POINTER == AS && DL.getPointerSizeInBits(AS) == 160)
+    return MVT::v8i32;
+  return AMDGPUTargetLowering::getPointerMemTy(DL, AS);
+}
+
 bool SITargetLowering::getTgtMemIntrinsic(IntrinsicInfo &Info,
                                           const CallInst &CI,
                                           MachineFunction &MF,
                                           unsigned IntrID) const {
   Info.flags = MachineMemOperand::MONone;
   if (CI.hasMetadata(LLVMContext::MD_invariant_load))
     Info.flags |= MachineMemOperand::MOInvariant;
 

llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-non-integral-address-spaces-vectors.ll

+; RUN: not --crash llc -global-isel -mtriple=amdgcn-amd-amdpal -mcpu=gfx900 -o - -stop-after=irtranslator < %s
+
+; Confirm that no one's gotten vectors of addrspace(7) pointers to go through the
+; IR translater incidentally.
+
+define <2 x ptr addrspace(7)> @no_auto_constfold_gep_vector() {
+  %gep = getelementptr i8, <2 x ptr addrspace(7)> zeroinitializer, <2 x i32> <i32 123, i32 123>
+  ret <2 x ptr addrspace(7)> %gep
+}
+
+define <2 x ptr addrspace(7)> @gep_vector_splat(<2 x ptr addrspace(7)> %ptrs, i64 %idx) {
+  %gep = getelementptr i8, <2 x ptr addrspace(7)> %ptrs, i64 %idx
+  ret <2 x ptr addrspace(7)> %gep
+}

llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-non-integral-address-spaces.ll

+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+; RUN: llc -global-isel -mtriple=amdgcn-amd-amdpal -mcpu=gfx900 -o - -stop-after=irtranslator %s | FileCheck %s
+
+; Check that the CSEMIRBuilder doesn't fold away the getelementptr during IRTranslator
+define ptr addrspace(7) @no_auto_constfold_gep() {
+  ; CHECK-LABEL: name: no_auto_constfold_gep
+  ; CHECK: bb.1 (%ir-block.0):
+  ; CHECK-NEXT:   [[C:%[0-9]+]]:_(p7) = G_CONSTANT i160 0
+  ; CHECK-NEXT:   [[C1:%[0-9]+]]:_(s160) = G_CONSTANT i160 123
+  ; CHECK-NEXT:   [[PTR_ADD:%[0-9]+]]:_(p7) = G_PTR_ADD [[C]], [[C1]](s160)
+  ; CHECK-NEXT:   [[UV:%[0-9]+]]:_(s32), [[UV1:%[0-9]+]]:_(s32), [[UV2:%[0-9]+]]:_(s32), [[UV3:%[0-9]+]]:_(s32), [[UV4:%[0-9]+]]:_(s32) = G_UNMERGE_VALUES [[PTR_ADD]](p7)
+  ; CHECK-NEXT:   $vgpr0 = COPY [[UV]](s32)
+  ; CHECK-NEXT:   $vgpr1 = COPY [[UV1]](s32)
+  ; CHECK-NEXT:   $vgpr2 = COPY [[UV2]](s32)
+  ; CHECK-NEXT:   $vgpr3 = COPY [[UV3]](s32)
+  ; CHECK-NEXT:   $vgpr4 = COPY [[UV4]](s32)
+  ; CHECK-NEXT:   SI_RETURN implicit $vgpr0, implicit $vgpr1, implicit $vgpr2, implicit $vgpr3, implicit $vgpr4
+  %gep = getelementptr i8, ptr addrspace(7) null, i32 123
+  ret ptr addrspace(7) %gep
+}

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

+; 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
+}
+
+define void @f2(<2 x ptr addrspace(7)> %arg) {
+; CHECK-LABEL: define void @f2
+; CHECK-SAME: (<2 x ptr addrspace(7)> [[ARG:%.*]]) {
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    br label [[BB1:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[P:%.*]] = extractelement <2 x ptr addrspace(7)> [[ARG]], i32 0
+; CHECK-NEXT:    [[I:%.*]] = getelementptr i32, ptr addrspace(7) [[P]], i32 2
+; CHECK-NEXT:    br i1 false, label [[BB2:%.*]], label [[BB1]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[I_LCSSA:%.*]] = phi ptr addrspace(7) [ [[I]], [[BB1]] ]
+; CHECK-NEXT:    br label [[BB3:%.*]]
+; CHECK:       bb3:
+; CHECK-NEXT:    [[I4:%.*]] = load i32, ptr addrspace(7) [[I_LCSSA]], align 4
+; CHECK-NEXT:    br label [[BB3]]
+;
+bb:
+  br label %bb1
+bb1:
+  %p = extractelement <2 x ptr addrspace(7)> %arg, i32 0
+  %i = getelementptr i32, ptr addrspace(7) %p, 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
+}