[PATCH] D124463: [RISCV] Add isCommutable to scalar FMA instructions.

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

In D124463#3475162 <https://reviews.llvm.org/D124463#3475162>, @craig.topper wrote:

> In D124463#3475151 <https://reviews.llvm.org/D124463#3475151>, @reames wrote:
>
>> Declaring a three operand opcode as commutative when only two of the arguments can be safely commuted feels rather suspect to me.
>
> The commutable operands are determined by TargetInstrInfo::findCommutedOpIndices. The default implementation picks the first two source operands. X86 relies on this for commuting AMD's FMA4 instructions. For the FMA3 instructions, X86 overrides TargetInstrInfo::findCommutedOpIndices and calls X86InstrInfo::findThreeSrcCommutedOpIndices. An additional override of TargetInstrInfo::commuteInstructionImpl is used to change the opcode for FMA3.
>
> Should I add more tests or more comments or both?

I'm not arguing correctness of the change, just that it is quite confusingly structured.  However, I went and dug through the existing code as you suggested, and you're definitely right that backends rely on the first-two-non-def-operands thing already.  This isn't adding new weirdness, it's just matching what everyone else already does.

Given that, LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124463