[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#2230458 <https://reviews.llvm.org/D86203#2230458>, @arsenm 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.
>
> As I mentioned, the VS_32 is unallocatable and only exists to model the operand constraints. The actual operands should be either VGPR_32 or SGPR_32. Querying the register bank bank for VS_32 doesn't make any sense, and I suspect would assert.
>
> 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.
>
>> 
>
> This would produce a broken import by default with unannotated patterns, which is undesirable. This still has the same problem before, but now all the patterns are broken and it's harder to find which ones need fixing.
>
>> 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?
>
> I think tablegen is hard enough to understand as is, and changing behaviors based on targets would make that problem worse.

If we could make this so that it only applies in situations where what is already annotated to the instruction operand is the only sensable option for the pattern opernads - i.e. only do default annotation when the pattern operand is a leaf and the register class of the instruction operand is (1) allocatable, and (2) not a union of two other register classes - would that be acceptable? I agree that we should not create broken imports nor make things harder to debug, but if we can automate things in a safe manner, why not do it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86203