[PATCH] D96567: [RISCV] Add support for fixed vector floating point setcc.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

Looks good on the whole, just some minor comments. Shame about having to do our own CC legalization though.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2103
+  bool Invert = false;
+  unsigned LogicOpc = ISD::DELETED_NODE;
+  if (ContainerVT.isFloatingPoint()) {
----------------
I'd probably use `Optional<unsigned>` for this but maybe that's personal preference.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2158
+
+  if (LogicOpc != ISD::DELETED_NODE && CC != ISD::SETOLT) {
+    Cmp = DAG.getNode(RISCVISD::SETCC_VL, DL, MaskVT, Op1, Op1,
----------------
I think this block could be commented, like why is SETOLT special.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td:84
+                            SDTypeProfile<1, 5, [SDTCisVec<0>,
+                                                 SDTCVecEltisVT<0, i1>,
+                                                 SDTCisVec<1>,
----------------
I was wondering if this warrants a `SDTCisVecWithSameNumEltsAndEltVT<opidx, otheropidx, eltvt>` to combine these three commonly-used constraints (`SDTCisVec<1>, SDTCisSameNumEltsAs<0, 1>, SDTCVecEltisVT<0, X>`) but the name alone gives me second thoughts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96567