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

[Simon Moll via Phabricator via llvm-commits]

simoll added a comment.

In D121292#3372302 <https://reviews.llvm.org/D121292#3372302>, @frasercrmck wrote:

> 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.

I agree, it's verbose and there is no other comparison operators in sight.. but having macros in VPIntrinsics.def is the canonical form, so i am still in favor of going the `VP_PROPERTY_CMP` route.
+1 for `VPCmp`. If needed that can be specialized later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121292