[PATCH] D123051: [RISCV][VP] Add basic RVV codegen for vp.fcmp

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

LGTM other than the assert and the comments about the IsVP computation.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:9211
+  assert((EVL != SDValue() || Mask == SDValue()) &&
+         "VP Mask and EVL must either both be set or unset");
+  bool IsVP = EVL != SDValue();
----------------
I don't think this is assert is complete. It's only checking that Mask is null when EVL is null. If EVL is non-null it doesn't check Mask.

Something like assert(!EVL == !Mask) would check both conditions.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:9212
+         "VP Mask and EVL must either both be set or unset");
+  bool IsVP = EVL != SDValue();
   switch (TLI.getCondCodeAction(CCCode, OpVT)) {
----------------
I'm debating whether we need the "!= SDValue()" SDValue has an operator bool. But `bool isVP = EVL` might not be very readable. There's also `bool isVP = !!EVL` but I don't know if that's any better.

I only found 1 file in tree ARMISelLowering.cpp that does `== SDValue()` or `!= SDValue()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123051