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

[Gabriel Hjort Åkerlund via Phabricator via llvm-commits]

ehjogab added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86203