-
Notifications
You must be signed in to change notification settings - Fork 18.1k
[LV][EVL] Support cast instruction with EVL-vectorization #108351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
28e4d5c
4d3be15
3f4e0e5
cc04719
363d625
0e45dc4
1d8f27c
5bd40b9
118f55a
6b8fb80
1e54505
b7bc8a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -962,22 +962,35 @@ void VPWidenIntrinsicRecipe::execute(VPTransformState &State) { | |
|
|
||
| // Use vector version of the intrinsic. | ||
| Module *M = State.Builder.GetInsertBlock()->getModule(); | ||
| bool IsVPIntrinsic = VPIntrinsic::isVPIntrinsic(VectorIntrinsicID); | ||
| Function *VectorF = | ||
| Intrinsic::getOrInsertDeclaration(M, VectorIntrinsicID, TysForDecl); | ||
| assert(VectorF && "Can't retrieve vector intrinsic."); | ||
| IsVPIntrinsic | ||
| ? VPIntrinsic::getOrInsertDeclarationForParams(M, VectorIntrinsicID, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, like llvm.vp.fptoint
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @LiqinWeng Yes, that is a problem.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| TysForDecl[0], Args) | ||
| : Intrinsic::getOrInsertDeclaration(M, VectorIntrinsicID, TysForDecl); | ||
| assert(VectorF && | ||
| "Can't retrieve vector intrinsic or vector-predication intrinsics."); | ||
|
|
||
| auto *CI = cast_or_null<CallInst>(getUnderlyingValue()); | ||
| SmallVector<OperandBundleDef, 1> OpBundles; | ||
| if (CI) | ||
| CI->getOperandBundlesAsDefs(OpBundles); | ||
| if (!IsVPIntrinsic) { | ||
| if (auto *CI = cast_or_null<CallInst>(getUnderlyingValue())) | ||
| CI->getOperandBundlesAsDefs(OpBundles); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment on why this is isn't supported for VPINstrinsics.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
|
|
||
| CallInst *V = State.Builder.CreateCall(VectorF, Args, OpBundles); | ||
| Instruction *V = State.Builder.CreateCall(VectorF, Args, OpBundles); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| setFlags(V); | ||
| if (IsVPIntrinsic) { | ||
| // Currently vp-intrinsics only accept FMF flags. llvm.vp.uitofp will get | ||
| // Flags of OperationType::NonNegOp && OperationType::FPMathOp. | ||
| if (isa<FPMathOperator>(V) && VectorIntrinsicID != Intrinsic::vp_uitofp) | ||
| setFlags(V); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we instead drop the flags on construction for widen intrinsics recipes wit EVL?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a question here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In the previous commit I pass the underlying instruction. At present, it seems that the underlying instruction information needs to be passed when setting metedata and flag. I am not sure about the flags of VP intrinsics, and they do not seem to correspond to non-vp intrinsics. For example, uitofp has the nneg IR flags, but llvm.vp.uitofp seems no this flag. I'm not sure if I understood something wrong. :) . And can we temporarily not set flag and resubmit a new ptach to deal with it?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. vp.cast does not seem to require setting flags. So I dont pass the underlying instruction for cast transform to vp.cast . I am not sure if I understand it correctly. If there is any mistake, please point it out. |
||
| } else { | ||
| setFlags(V); | ||
| } | ||
|
|
||
| if (!V->getType()->isVoidTy()) | ||
| State.set(this, V); | ||
| State.addMetadata(V, CI); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change still need?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The following patch will be used, I can remove it first. Pls give LGTM :) |
||
| State.addMetadata(V, dyn_cast_or_null<Instruction>(getUnderlyingValue())); | ||
| } | ||
|
|
||
| InstructionCost VPWidenIntrinsicRecipe::computeCost(ElementCount VF, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1445,7 +1445,8 @@ static void transformRecipestoEVLRecipes(VPlan &Plan, VPValue &EVL) { | |
| VPTypeAnalysis TypeInfo(CanonicalIVType); | ||
| LLVMContext &Ctx = CanonicalIVType->getContext(); | ||
| SmallVector<VPValue *> HeaderMasks = collectAllHeaderMasks(Plan); | ||
|
|
||
| VPValue *AllOneMask = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also use for select VPInstruction below?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A separate NFC patch would be better :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I don't understand what you mean, the vp.select does not have a mask parameter.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think Florian means vp.merge.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will create a new patch to deal with it. |
||
| Plan.getOrAddLiveIn(ConstantInt::getTrue(IntegerType::getInt1Ty(Ctx))); | ||
| for (VPUser *U : Plan.getVF().users()) { | ||
| if (auto *R = dyn_cast<VPReverseVectorPointerRecipe>(U)) | ||
| R->setOperand(1, &EVL); | ||
|
|
@@ -1492,14 +1493,30 @@ static void transformRecipestoEVLRecipes(VPlan &Plan, VPValue &EVL) { | |
| assert(VPIntrinsic::getMaskParamPos(VPID) && | ||
| VPIntrinsic::getVectorLengthParamPos(VPID) && | ||
| "Expected VP intrinsic"); | ||
| VPValue *Mask = Plan.getOrAddLiveIn(ConstantInt::getTrue( | ||
| IntegerType::getInt1Ty(CI->getContext()))); | ||
| Ops.push_back(Mask); | ||
| Ops.push_back(AllOneMask); | ||
| Ops.push_back(&EVL); | ||
| return new VPWidenIntrinsicRecipe( | ||
| *CI, VPID, Ops, TypeInfo.inferScalarType(CInst), | ||
| CInst->getDebugLoc()); | ||
| }) | ||
| .Case<VPWidenCastRecipe>( | ||
| [&](VPWidenCastRecipe *CInst) -> VPRecipeBase * { | ||
| auto *CI = dyn_cast<CastInst>(CInst->getUnderlyingInstr()); | ||
| Intrinsic::ID VPID = | ||
| VPIntrinsic::getForOpcode(CI->getOpcode()); | ||
| if (VPID == Intrinsic::not_intrinsic) | ||
| return nullptr; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what casts are not supported? Do we have tests for them?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not found yet
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make it an assertion then?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
|
||
| SmallVector<VPValue *> Ops(CInst->operands()); | ||
| assert(VPIntrinsic::getMaskParamPos(VPID) && | ||
| VPIntrinsic::getVectorLengthParamPos(VPID) && | ||
| "Expected VP intrinsic"); | ||
| Ops.push_back(AllOneMask); | ||
| Ops.push_back(&EVL); | ||
| return new VPWidenIntrinsicRecipe( | ||
| VPID, Ops, TypeInfo.inferScalarType(CInst), | ||
| CInst->getDebugLoc()); | ||
| }) | ||
| .Case<VPWidenSelectRecipe>([&](VPWidenSelectRecipe *Sel) { | ||
| SmallVector<VPValue *> Ops(Sel->operands()); | ||
| Ops.push_back(&EVL); | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change still need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably sufficient to keep the existing message