[PATCH] D128310: [RISCV] Move vfma_vl+fneg_vl matching to DAG combine.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Direction looks reasonable, a couple comments.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8480
+static unsigned negateFMAOpcode(unsigned Opcode, bool NegMul, bool NegAcc) {
+  if (NegMul) {
+    // clang-format off
----------------
The logic flow here is very confusing.  In particular, I can't quite tell if this handles the case where we invert both or not.

I'd suggest structuring this with an explicit if (NegMul && NegAcc) case, and then use early return.

As a further idea, what we really have here is for each case, pairs of inverted opcodes.  Grouping the code that way would help readability.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td:1242
   defvar suffix = vti.LMul.MX;
-  def : Pat<(vti.Vector (riscv_fma_vl vti.RegClass:$rs1, vti.RegClass:$rd,
-                                      vti.RegClass:$rs2, (vti.Mask true_mask),
-                                      VLOpFrag)),
+  def : Pat<(vti.Vector (riscv_vfmadd_vl vti.RegClass:$rs1, vti.RegClass:$rd,
+                                         vti.RegClass:$rs2, (vti.Mask true_mask),
----------------
It looks like after your change, these become regular across the four opcodes, can we use a pattern class here to reduce code duplication?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128310