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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
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)
----------------
frasercrmck wrote:
> 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?
Let's remove it from this patch.

That FIXME on fcmp fast math flgs has been there for a while. I don't think we're close to fixing it yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122743