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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked an inline comment as done.
frasercrmck 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)
----------------
craig.topper wrote:
> 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.
Alright, sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122743