This is an archive of the discontinued LLVM Phabricator instance.

[VP] Add vp.fcmp comparison intrinsic and docs
ClosedPublic

Authored by frasercrmck on Mar 9 2022, 7:16 AM.

Details

Summary

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.

Diff Detail

Event Timeline

frasercrmck created this revision.Mar 9 2022, 7:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2022, 7:16 AM
frasercrmck requested review of this revision.Mar 9 2022, 7:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2022, 7:16 AM

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

frasercrmck marked an inline comment as done.Mar 10 2022, 3:31 AM

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.

llvm/docs/LangRef.rst
20304

yep, totally. bad copy/paste from the constrained.fcmp docs. thanks!

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.

frasercrmck marked an inline comment as done.

add VP_PROPERTY_CMP

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.

add attributes: speculatable,nomem,willreturn, etc

simoll added a comment.EditedMar 15 2022, 3:43 AM

VP intrinsics aren't speculatable because they have undefined behavior if %evl > static vector length

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.

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.

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.

  • rebase and drop Speculatable attribute
simoll added inline comments.Mar 23 2022, 7:01 AM
llvm/include/llvm/IR/Intrinsics.td
1572

Does DefaultAttrsIntrinsic imply speculatable ?

frasercrmck marked an inline comment as done.Mar 23 2022, 7:22 AM
frasercrmck added inline comments.
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.

This revision is now accepted and ready to land.Mar 29 2022, 3:06 PM
frasercrmck marked an inline comment as done.
  • rebase and fix HandleToConstrainedFP unit test

fix a grammar issue and the mangling used in a test

This revision was landed with ongoing or failed builds.Mar 30 2022, 6:51 AM
This revision was automatically updated to reflect the committed changes.