[AMDGPU] Fix handling of void types in isLegalAddressingMode

[rampitec]

Summary

It is legal for the type passed to isLegalAddressingMode to be unsized or, more specifically, VoidTy. In this case, we must check the legality of load / stores for all legal types. Directly trying to call getTypeStoreSize is incorrect, and leads to breakage in e.g. Loop Strength Reduction. This change guards against that behaviour.

[arsenm]

Needs test

[rampitec]

lib/Target/AMDGPU/SIISelLowering.cpp

679 ↗

(On Diff #124117)

I would prefer to exit earlier than this. It is not address space specific and we hardly can do anything good on a void or opaque type.

[rampitec]

lib/Target/AMDGPU/SIISelLowering.cpp

679 ↗

(On Diff #124117)

... but then it crashes another couple of tests. We seems to be required to process that LSR formulae.

[AlexVlx]

I believe that we need to behave as is LegalAddressingMode is supposed to behave in general(see 469 in TargetTransformInfo.h):

/// Return true if the addressing mode represented by AM is legal for
/// this target, for a load/store of the specified type.
/// The type may be VoidTy, in which case only return true if the addressing
/// mode is legal for a load/store of any legal type.
/// If target returns true in LSRWithInstrQueries(), I may be valid.

When I looked at it, it was the case that VoidTy is essentially a tag type in this case. Other targets such as AArch64 also acknowledge that an unsized type may be passed through all the way to isLegalAddressingMode.

[rampitec]

Added test.

[AlexVlx]

LGTM - thank you!

[arsenm]

Test should go in test/LoopStrengthReduce/AMDGPU and run just LSR

[rampitec]

In D40405#1100698, @arsenm wrote:

Test should go in test/LoopStrengthReduce/AMDGPU and run just LSR

I can move the test, but I cannot reproduce the bug with opt.

[rampitec]

In D40405#1102009, @rampitec wrote:

In D40405#1100698, @arsenm wrote:

Test should go in test/LoopStrengthReduce/AMDGPU and run just LSR

I can move the test, but I cannot reproduce the bug with opt.

https://reviews.llvm.org/rL332562

[arsenm]

In D40405#1102009, @rampitec wrote:

In D40405#1100698, @arsenm wrote:

Test should go in test/LoopStrengthReduce/AMDGPU and run just LSR

I can move the test, but I cannot reproduce the bug with opt.

You should only change the test if the original problem shows up. You should be able to coax it into failing with just opt though (maybe you need to run more passes before LSR on it?)

[rampitec]

In D40405#1102663, @arsenm wrote:

In D40405#1102009, @rampitec wrote:

In D40405#1100698, @arsenm wrote:

Test should go in test/LoopStrengthReduce/AMDGPU and run just LSR

I can move the test, but I cannot reproduce the bug with opt.

You should only change the test if the original problem shows up. You should be able to coax it into failing with just opt though (maybe you need to run more passes before LSR on it?)

Test is not changed, just moved. Note that many of our lsr tests there use llc to run. I was unable to make it reproducible with opt given any pass set. If one wants to run it with opt it will probably require a different test.

llvm/trunk/lib/Target/AMDGPU/SIISelLowering.cpp

if (AS == AMDGPUASI.CONSTANT_ADDRESS ||
     // FIXME: Can we get the real alignment here?
     if (AM.BaseOffs % 4 != 0)
       return isLegalMUBUFAddressingMode(AM);
 
     // There are no SMRD extloads, so if we have to do a small type access we
     // will use a MUBUF load.
     // FIXME?: We also need to do this if unaligned, but we don't know the
     // alignment here.
-    if (DL.getTypeStoreSize(Ty) < 4)
+    if (Ty->isSized() && DL.getTypeStoreSize(Ty) < 4)
       return isLegalGlobalAddressingMode(AM);
 
     if (Subtarget->getGeneration() == SISubtarget::SOUTHERN_ISLANDS) {
       // SMRD instructions have an 8-bit, dword offset on SI.
       if (!isUInt<8>(AM.BaseOffs / 4))
         return false;
     } else if (Subtarget->getGeneration() == SISubtarget::SEA_ISLANDS) {
       // On CI+, this can also be a 32-bit literal constant offset. If it fits

llvm/trunk/test/CodeGen/AMDGPU/lsr-void.ll

+; RUN: llc -march=amdgcn < %s | FileCheck -check-prefix=GCN %s
+
+@array = external addrspace(4) constant [32 x [800 x i32]], align 4
+
+; GCN-LABEL: {{^}}test_lsr_voidty:
+define amdgpu_kernel void @test_lsr_voidty() {
+entry:
+  br label %for.body
+
+for.body:                                 ; preds = %for.body.i, %entry
+  br label %for.body.i
+
+for.body.i:                               ; preds = %for.body.i, %for.body
+  %ij = phi i32 [ 0, %for.body ], [ %inc14, %for.body.i ]
+  %tmp = load i32, i32 addrspace(5)* undef, align 4
+  %inc13 = or i32 %ij, 2
+  %shl = shl i32 1, 0
+  %and = and i32 %shl, %tmp
+  %tobool = icmp eq i32 %and, 0
+  %add = mul nuw nsw i32 %inc13, 5
+  %tmp1 = zext i32 %add to i64
+  %arrayidx8 = getelementptr inbounds [32 x [800 x i32]], [32 x [800 x i32]] addrspace(4)* @array, i64 0, i64 undef, i64 %tmp1
+  %tmp2 = load i32, i32 addrspace(4)* %arrayidx8, align 4
+  %and9 = select i1 %tobool, i32 0, i32 %tmp2
+  %xor = xor i32 undef, %and9
+  %inc1 = or i32 %ij, 3
+  %add2 = mul nuw nsw i32 %inc1, 5
+  %add6 = add nuw nsw i32 %add2, 1
+  %tmp3 = zext i32 %add6 to i64
+  %arrayidx9 = getelementptr inbounds [32 x [800 x i32]], [32 x [800 x i32]] addrspace(4)* @array, i64 0, i64 undef, i64 %tmp3
+  %tmp4 = bitcast i32 addrspace(4)* %arrayidx9 to <4 x i32> addrspace(4)*
+  %tmp5 = load <4 x i32>, <4 x i32> addrspace(4)* %tmp4, align 4
+  %reorder_shuffle2 = shufflevector <4 x i32> %tmp5, <4 x i32> undef, <4 x i32> <i32 3, i32 2, i32 1, i32 0>
+  %tmp6 = select <4 x i1> undef, <4 x i32> zeroinitializer, <4 x i32> %reorder_shuffle2
+  %inc14 = add nuw nsw i32 %ij, 4
+  br i1 undef, label %for.body, label %for.body.i
+}