[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

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

... 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.

lib/Target/AMDGPU/SIISelLowering.cpp

     // 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