[PATCH] D122743: [RISCV][VP] Add basic RVV codegen for vp.icmp

[Fraser Cormack via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAG.h:1081
+                     ISD::CondCode Cond, SDValue Mask, SDValue EVL) {
+    assert(LHS.getValueType().isVector() == RHS.getValueType().isVector() &&
+           "Cannot compare scalars to vectors");
----------------
craig.topper wrote:
> This could be stricter. Everything needs to be a vector.
Yep, good catch.


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:271
 // llvm.vp.icmp(x,y,cc,mask,vlen)
 BEGIN_REGISTER_VP(vp_icmp, 3, 4, VP_ICMP, -1)
 VP_PROPERTY_FUNCTIONAL_OPC(ICmp)
----------------
rogfer01 wrote:
> Now that we're at it, could we replace this block with the following one?
> 
> ```lang=cpp
> BEGIN_REGISTER_VP_INTRINSIC(vp_icmp, 3, 4)
> VP_PROPERTY_FUNCTIONAL_OPC(ICmp)
> VP_PROPERTY_CMP(2, false)
> END_REGISTER_VP_INTRINSIC(vp_icmp)
> ```
> 
> Should be equivalent and avoids defining an unnecessary `ISD::VP_ICMP` which we are not going to use.
> 
> (A similar thing can be done for `VP_FCMP` but we can do that in the patch with the lowering)
Good shout, thanks.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7513
+    Condition = getFCmpCondCode(CondCode);
+    auto *FPMO = dyn_cast<FPMathOperator>(&VPIntrin);
+    if ((FPMO && FPMO->hasNoNaNs()) || TM.Options.NoNaNsFPMath)
----------------
craig.topper wrote:
> Is the vp.fcmp intrinsic considered an FPMathOperator or not? I wouldn't expect it to be dynamic. I'm guess it's not because it doesn't return an FP type which is what the dyn_cast is checking
I'd imagine that `vp.fcmp` should in theory be as much a FPMathOperator as `fcmp` is, but you're right about this never actually being a `FPMathOperator` due to the return type. I copied this particular code from the reference patch so I don't know if @simoll has other ideas. Looking again at the reference patch, I notice a change to `classof`  to support VP intrinsics:

```
    // Judge based on function.
    auto *VPIntrin = dyn_cast<VPIntrinsic>(V);
    if (VPIntrin) {
      auto OCOpt = VPIntrin->getFunctionalOpcode();
      Opcode = OCOpt ? *OCOpt : (unsigned) Instruction::Call;
    }
```

Also `fcmp` being a `FPMathOperator` seems to be questionable:
```
    // FIXME: To clean up and correct the semantics of fast-math-flags, FCmp
    //        should not be treated as a math op, but the other opcodes should.
    //        This would make things consistent with Select/PHI (FP value type
    //        determines whether they are math ops and, therefore, capable of
    //        having fast-math-flags).
    case Instruction::FCmp:
      return true;
```

My initial reaction is to remove this bit for now and come back to it? This patch is only nominally supporting `icmp` anyway. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122743