This patch adds the first support for vector-predicated comparison
intrinsics, starting with vp.fcmp. It uses metadata to encode its
condition code, like the llvm.experimental.constrained.fcmp intrinsic.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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; }
llvm/docs/LangRef.rst | ||
---|---|---|
20304 | Should only be a vector of boolean right? |
fix langref doc on return value
add VPCmpIntrinsic and verification of predicate
add testing for predicate verification
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.
llvm/docs/LangRef.rst | ||
---|---|---|
20304 | yep, totally. bad copy/paste from the constrained.fcmp docs. thanks! |
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.
Fair enough, that makes sense. I've added VP_PROPERTY_CMP and am using it for the CC operand position and true/false about whether it takes FP condition codes. Just so we're not partially relying on a property and partially on explicit opcode checks when integer comparisons come around.
VP intrinsics aren't speculatable because they have undefined behavior if %evl > static vector length
Hmm, this sort of goes against what was said in 9b99927618d338a2be3f13053ccf869a39eb1637.
Right. That patch is wrong then. What we really need is something that tells us whether the replacement for the VP intrinsic is speculatable (not the VP intrinsic itself).
Something like isSafeToSpeculativelyExecute with just the opcode and the operands.
Yeah, that makes sense. That needs to be a separate patch, however, so at the very least I should not be adding "speculatable" to these new intrinsics. This only really comes up as a problem in D121594.
llvm/include/llvm/IR/Intrinsics.td | ||
---|---|---|
1572 | Does DefaultAttrsIntrinsic imply speculatable ? |
llvm/include/llvm/IR/Intrinsics.td | ||
---|---|---|
1572 | No, shouldn't do, anyway. It just adds those attributes marked as being applied by default, which is done through IsDefault in IntrinsicProperty:, e.g., class IntrinsicProperty<bit is_default = false> { bit IsDefault = is_default; } // Applied by default. def IntrWillReturn : IntrinsicProperty<1>; // This property indicates that the intrinsic is safe to speculate. def IntrSpeculatable : IntrinsicProperty; This does mean that IntrNoSync and IntrWillReturn are redundant, but since all other VP intrinsics (and even others like masked_load) explicitly list them, I thought I'd keep things consistent until we decide to clean things up. |
Should only be a vector of boolean right?