[PATCH] D151850: [RISCV] Model all 3 arithmetic sources of vector FMA at MC layer.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

I think cleaning up `HasMergeOp` makes sense. If we're now adding `let HasMergeOp = 0` in various places it's telling me the design of our tablegen classes isn't working well.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:3190
   uint64_t TSFlags = TII.get(Opc).TSFlags;
-  assert((UseTUPseudo == RISCVII::hasMergeOp(TSFlags)) &&
+  assert(((UseTUPseudo == RISCVII::hasMergeOp(TSFlags)) ||
+          I->UnmaskedTUPseudo == I->UnmaskedPseudo) &&
----------------
Do we need the extra parens on this first term (which was there before)?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvfbf.td:29
+let Predicates = [HasStdExtZvfbfwma],
+    Constraints = "@earlyclobber $vd_wb, $vd = $vd_wb",
     RVVConstraint = WidenV, Uses = [FRM], mayRaiseFPException = true in {
----------------
I wonder if it's worth having `ExtraConstraints` which are automatically appended to `Constraints`, to avoid us having to duplicate the "inherent" tied operand constraints that the base class should be setting.

I don't know how practical that is given our class hierarchies, just a thought. It might make things harder to understand, not easier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151850