[PATCH] D134961: [AMDGPU][MC][GFX11] Correct v_fmac_.*_e64_dpp

[Joe Nash via Phabricator via llvm-commits]

Joe_Nash added a comment.

In D134961#3831092 <https://reviews.llvm.org/D134961#3831092>, @rampitec wrote:

> In D134961#3830760 <https://reviews.llvm.org/D134961#3830760>, @Joe_Nash wrote:
>
>> In D134961#3830721 <https://reviews.llvm.org/D134961#3830721>, @dp wrote:
>>
>>>> If I understand the current state: e32 dpp is not creatable but is assemblable, e64 dpp is  creatable but not assemblable.
>>>
>>> Correct. I'm not sure e64 dpp is creatable but at least it has $old operand.
>>>
>>>> Perhaps a way to fix _e64 is in the asmParser in cvtVOP3DPP ~L8900? I think it's treating the operand as src2 when it is actually the old operand.
>>>
>>> cvtVOP3DPP incorrectly handles $old because it is not tied to $dst.
>>>
>>> It seems possible to change cvtVOP3DPP to handle v_fmac opcodes correctly. But if we go that way, $src2 will be tied to $dst, but $old will be present but not tied. Is this acceptable?
>>
>> I believe it is actually that way currently in main. And it seems to work in GCNDPPCombine at least. Though I have not strongly tested it, maybe it's wasting a register? @rampitec
>>
>> Tablegen dump yields : 
>> def V_FMAC_F32_e64_dpp
>>
>>   ...
>>   dag InOperandList = (ins anonymous_9483:$old ...
>>   ...
>>   string Constraints = "$vdst = $src2";
>>   string DisableEncoding = "$src2";
>
> Do we really need both? src2 and old are basically the same thing here.

I think the most correct thing to do is remove src2 operand from FMAC. But the FIXME at L410 implies we had good reason for adding src2. If we don't want to do that, we should either fix GCNDPPCombine to work without an old operand for these instruction, or fix the Asm/Dasm to work for them.


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

https://reviews.llvm.org/D134961