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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 2 inline comments as done.
frasercrmck added inline comments.


================
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();
----------------
craig.topper wrote:
> 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.
Right you are, thanks!


================
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)) {
----------------
craig.topper wrote:
> 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()`
Yeah I'd considered all of those but wasn't happy with any one of them. It's a good point that `==/!= SDValue()` isn't common enough to warrant its usage though. I suppose `bool IsNonVP = !EVL` is both the more common situation and a (arguably) more common use of the operator bool, but then again even `= !X` is rare in the codebase.

I'll go with `IsNonVP` as it also matches the form now used in the assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123051