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

Jessica Clarke via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 13 10:08:59 PDT 2021


jrtc27 added a comment.

In D109032#2997871 <https://reviews.llvm.org/D109032#2997871>, @arsenm wrote:

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

Those examples may not be what you intend them to be, but I can promise you that if you go read AMDGPUGenDAGISel.inc yourself you will find that every single one of them is currently matching an iPTR. For example:

  // Src: (ld:{ *:[i32] } (ScratchOffset:{ *:[iPTR] } VGPR_32:{ *:[i32] }:$vaddr, i16:{ *:[i16] }:$offset))<<P:Predicate_unindexedload>><<P:Predicate_extload>><<P:Predicate_extloadi8_private>> - Complexity = 22
  // Dst: (SCRATCH_LOAD_UBYTE:{ *:[i32] } ?:{ *:[i32] }:$vaddr, ?:{ *:[i16] }:$offset)

is what I see *without* any of my patches, so I'm just preserving that. Things blow up with failures to select, or poor codegen that regresses tests, when these instead infer i32. If you want to see for yourself, apply D109035 <https://reviews.llvm.org/D109035>, note that the TableGen output changes to honour the existing intended types and that many AMDGPU tests fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109032



More information about the llvm-commits mailing list