[PATCH] D109032: [AMDGPU][NFC] Alter ComplexPattern types to be consistent with their uses

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D109032#2975958 <https://reviews.llvm.org/D109032#2975958>, @jrtc27 wrote:

> In D109032#2975943 <https://reviews.llvm.org/D109032#2975943>, @jrtc27 wrote:
>
>> In D109032#2975910 <https://reviews.llvm.org/D109032#2975910>, @arsenm wrote:
>>
>>> I'm not sure where this is going without the context of the other patch.
>>>
>>> I think iPTR is a flawed concept that doesn't work without the address space. AMDGPU does not want the type changing magic of iPTR when there are mixed and constant pointer sizes that never change, so the fixed types are more logical.
>>
>> I agree with you in a sense, it causes pain for us downstream to not have address spaces here (so we just add a new type for our other pointer representation downstream, and up until now it's been hacked together, but I'm trying to rewrite it to do things properly, and adding a new possible pointer type everywhere beyond iPTR causes a lot of believed-ambiguous-but-not patterns without better type inference in TableGen). However, if you look at the generated .inc files, you will see that all the cases where these are used TableGen has already inferred that iPTR is in use, and if I don't make this change but have my subsequent TableGen patch applied to actually propagate the types as they should have been from the start then the AMDGPU backend becomes very crash-prone with failures to select all over the place as it seems it really is expecting iPTR not i32/i64 for various things. The changes here have zero effect on the generated files without my TableGen patch, and ensure that even with my TableGen patch the output remains unchanged (otherwise they go from iPTR to i32/i64), so this doesn't change behaviour, it just makes the code more accurately state what is actually currently going on.
>
> For example, picking the first ComplexPattern here, MUBUFAddr64, I see 56 instances of `MUBUFAddr64:{` in AMDGPUGenDAGISel.inc, all of which are `MUBUFAddr64:{ *:[iPTR] }`. Changing the ComplexPattern to have type iPTR rather than i64 ensures that that remains the case once the ComplexPattern type is included in the type inference, rather than altering behaviour by inferring `MUBUFAddr64:{ *:[i64] }` instead (which is, I imagine, what the author _intended_ it to be, but not what it _actually_ is currently, and does not work if that's actually the type used).

The MUBUFAddr64 case isn't that interesting, since you're changing one form of "i64" to another form of "i64". I'm much more puzzled about how this is not blowing up on the cases that do use 32 bit pointers (e.g. all the DS* patterns)



================
Comment at: llvm/lib/Target/AMDGPU/FLATInstructions.td:9-10
 
-def FlatOffset : ComplexPattern<i64, 2, "SelectFlatOffset", [], [SDNPWantRoot], -10>;
-def GlobalOffset : ComplexPattern<i64, 2, "SelectGlobalOffset", [], [SDNPWantRoot], -10>;
-def ScratchOffset : ComplexPattern<i32, 2, "SelectScratchOffset", [], [SDNPWantRoot], -10>;
+def FlatOffset : ComplexPattern<iPTR, 2, "SelectFlatOffset", [], [SDNPWantRoot], -10>;
+def GlobalOffset : ComplexPattern<iPTR, 2, "SelectGlobalOffset", [], [SDNPWantRoot], -10>;
+def ScratchOffset : ComplexPattern<iPTR, 2, "SelectScratchOffset", [], [SDNPWantRoot], -10>;
----------------
I would believe these are NFC


================
Comment at: llvm/lib/Target/AMDGPU/FLATInstructions.td:11
+def GlobalOffset : ComplexPattern<iPTR, 2, "SelectGlobalOffset", [], [SDNPWantRoot], -10>;
+def ScratchOffset : ComplexPattern<iPTR, 2, "SelectScratchOffset", [], [SDNPWantRoot], -10>;
 
----------------
There's no way this is actually correct


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.td:1320-1322
+def DS1Addr1Offset : ComplexPattern<iPTR, 2, "SelectDS1Addr1Offset">;
+def DS64Bit4ByteAligned : ComplexPattern<iPTR, 3, "SelectDS64Bit4ByteAligned">;
+def DS128Bit8ByteAligned : ComplexPattern<iPTR, 3, "SelectDS128Bit8ByteAligned">;
----------------
arsenm wrote:
> I think iPTR is a flawed concept that doesn't work without the address space, so I don't think AMDGPU should ever use it
These are also i32, I don't see how this is working


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109032/new/

https://reviews.llvm.org/D109032