[PATCH] D121292: [VP] Add vp.fcmp comparison intrinsic and docs

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked an inline comment as done.
frasercrmck added a comment.

In D121292#3371029 <https://reviews.llvm.org/D121292#3371029>, @craig.topper wrote:

> Need a Verifier.cpp check on the validity of the metadata
>
> Equivalent for constrained intrinsics
>
>   case Intrinsic::experimental_constrained_fcmp:                                 
>   case Intrinsic::experimental_constrained_fcmps: {                              
>     auto Pred = cast<ConstrainedFPCmpIntrinsic>(&FPI)->getPredicate();           
>     Assert(CmpInst::isFPPredicate(Pred),                                         
>            "invalid predicate for constrained FP comparison intrinsic", &FPI);   
>     break;                                                                       
>   }  

Ah yes, I got thrown off because there's surprisingly no testing for that so I missed it. I've added that now.

I wasn't 100% on whether we want //both// `VPIntCmp` and `VPFPCmp` classes so I went with a general `VPCmp` base class, for now at least. It can fetch the `CmpInst::Predicate`, the idea being that the integer `vp.icmp` can use that too. I went for a simple opcode check for `classof` (rather than a `VP_PROPERTY_CMP` since I felt `getPredicate` would have to check the opcode anyway. Unless we use `VP_PROPERTY_FCMP` and `VP_PROPERTY_ICMP` but that seems overly verbose to me. I don't know what you think about that, @simoll.



================
Comment at: llvm/docs/LangRef.rst:20153
+
+The '``llvm.vp.fcmp``' intrinsic returns a boolean value or vector of boolean
+values based on the comparison of its operands. The operation has a mask and an
----------------
craig.topper wrote:
> Should only be a vector of boolean right?
yep, totally. bad copy/paste from the constrained.fcmp docs. thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121292