[PATCH] D99376: [AMDGPU] Mark additional VOP3 as commutable

Tue Mar 30 10:42:17 PDT 2021

rampitec added a comment.

In D99376#2659286 <https://reviews.llvm.org/D99376#2659286>, @Joe_Nash wrote:

> In D99376#2657778 <https://reviews.llvm.org/D99376#2657778>, @rampitec wrote:
>
>> In D99376#2656686 <https://reviews.llvm.org/D99376#2656686>, @Joe_Nash wrote:
>>
>>> Reverted since I don't fully understand the issue. It passed all tests on my system, but then upon rebasing top of tree it didn't. So I suspect the commutes may not be deterministic/ well constrained.
>>
>> I have a bad suspicion here: that is not OK to just commute source modifiers for opsel. A DST_OP_SEL shares bits with $src0_modifiers, so it needs to be transferred to the new src0 modifiers and masked in src1. I suspect it does not happen.
>
> Ok I see the issue. In general, how can we tell apart src0 with opsel=1 and dst with opsel=1?

I think the fix is not too difficult. If the opcode is opsel it has op_sel operand. Then on commute we need to transfer DST_OP_SEL bit from src0_modifiers to the new src0_modifiers and reset it in the new src1_modifiers. The rest should be OK.
The src0_modifiers has DST_OP_SEL for vdst bit and OP_SEL_0/OP_SEL_1 for the src0 itself.
You can always verify what you are getting if use llc -start-before instead of llc -run-pass. That will print the asm so you could see resulting op_sel easier. I have to admit I wish to have a more readable mir for that too.

> To avoid issues with commute, I would say don't commute vop3 with 16bit operands. However, V_MAD_I16 and V_MAD_I16_gfx9 do already have isCommute =1. Is this perhaps a bug?

These are commutable. The bug is that we don't properly handle it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99376