[PATCH] D86203: [GlobalISel][TableGen] Add handling of unannotated dst pattern ops

Sat Aug 22 00:25:34 PDT 2020

madhur13490 added a comment.

In D86203#2230218 <https://reviews.llvm.org/D86203#2230218>, @ehjogab wrote:

> In D86203#2230217 <https://reviews.llvm.org/D86203#2230217>, @madhur13490 wrote:
>
>> In D86203#2229816 <https://reviews.llvm.org/D86203#2229816>, @ehjogab wrote:
>>
>>> In D86203#2226535 <https://reviews.llvm.org/D86203#2226535>, @arsenm wrote:
>>>
>>>> In D86203#2226357 <https://reviews.llvm.org/D86203#2226357>, @ehjogab wrote:
>>>>
>>>>> In D86203#2226073 <https://reviews.llvm.org/D86203#2226073>, @arsenm wrote:
>>>>>
>>>>>> What happens if the instruction operand uses an unallocatable pseudo-class combining register classes with different banks? I would expect it would need to check any of the bank possibilities
>>>>>
>>>>> Hm, not sure what you mean. Could you provide a brief example?
>>>>
>>>> The most common AMDGPU instructions define VS_* pseudoclasses for operand constraints. The two classes added have different register banks, since the instructions support directly reading from either. 
>>>> e.g.
>>>> RegisterClass VS_32 = {VGPR_32RegClass, SGPR_32RegClass}, where both of these register classes are different banks.
>>>>
>>>> In terms of selecting a particular operand, there's an additional hazard. We can't freely use SGPR_32 in a given operand without considering the context of the other operands. We avoid solving this in GlobalISel by always assigning these operands to VGPRRegBankID, and relying on a later pass to optimize out these copies. We're sorely missing a RegBank legality verifier check, so I'm not sure I would want the generated patterns to infer either bank is OK.
>>>
>>> The operands of these VS_* instructions, are the defined (in the instruction) to use the VS_32 class, or VGPR_32/SGPR_32 classes? If it is the former, then what this patch would do is to use VS_32 in the patterns. If the patterns should really be using VGPR_32/SGPR_32, then this is a problem. However, you still have the option of annotating the pattern operands just as before, so this patch would not interfere with any patterns that today are accepted by TableGen. If it is the latter, then this patch would use whatever class (VGPR_32 or SGPR_32) that is specified for the corresponding operands in the instruction, so here I don't see there being any problem.
>>>
>>> Perhaps we could add an option that this feature is disabled by default, and by activating it you take responsibility to ensure that you don't have these kinds of situations for your target?
>>
>> When a register class is an union of two or more classes then such problems bound to occur for one or more targets. Why not do such inference when it is obvious i.e. register class in instruction definition is a leaf register class and NOT an union of some base other base classes. I think this is better than having an runtime option to enable/disable the inference. 
>> I think leaf classes can be detected programmatically.
>
> I like that idea, and this is only applied on leaf nodes to begin with.

If it is applied to leaf nodes in this patch then it shouldn't be a problem to our AMDGPU backend, isn't it @arsenm?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86203