-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[TTI] Don't drop VP intrinsic args when delegating to non-vp equivalent #147677
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
|
@@ -1781,18 +1781,25 @@ class BasicTTIImplBase : public TargetTransformInfoImplCRTPBase<T> { | |
assert(ICA.getArgTypes().size() >= 2 && | ||
"Expected VPIntrinsic to have Mask and Vector Length args and " | ||
"types"); | ||
|
||
ArrayRef<const Value *> NewArgs = ArrayRef(ICA.getArgs()); | ||
if (!ICA.isTypeBasedOnly()) | ||
NewArgs = NewArgs.drop_back(2); | ||
ArrayRef<Type *> NewTys = ArrayRef(ICA.getArgTypes()).drop_back(2); | ||
|
||
// VPReduction intrinsics have a start value argument that their non-vp | ||
// counterparts do not have, except for the fadd and fmul non-vp | ||
// counterpart. | ||
if (VPReductionIntrinsic::isVPReduction(ICA.getID()) && | ||
*FID != Intrinsic::vector_reduce_fadd && | ||
*FID != Intrinsic::vector_reduce_fmul) | ||
*FID != Intrinsic::vector_reduce_fmul) { | ||
if (!ICA.isTypeBasedOnly()) | ||
NewArgs = NewArgs.drop_front(); | ||
NewTys = NewTys.drop_front(); | ||
} | ||
Comment on lines
1793
to
+1799
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 code covered? Maybe I'm missing something, but isn't vp.is.fpclass a non-reduction VP intrinsic? 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. Yeah it's covered by the existing tests at llvm/test/Analysis/CostModel/RISCV/reduce-fadd/fmul.ll. It just so happens that there's no test diff with that change because the type-based and value-based costing is the same for these intrinsics, i.e. here's the code for the value costing path: case Intrinsic::vector_reduce_add:
case Intrinsic::vector_reduce_mul:
case Intrinsic::vector_reduce_and:
case Intrinsic::vector_reduce_or:
case Intrinsic::vector_reduce_xor:
case Intrinsic::vector_reduce_smax:
case Intrinsic::vector_reduce_smin:
case Intrinsic::vector_reduce_fmax:
case Intrinsic::vector_reduce_fmin:
case Intrinsic::vector_reduce_fmaximum:
case Intrinsic::vector_reduce_fminimum:
case Intrinsic::vector_reduce_umax:
case Intrinsic::vector_reduce_umin: {
IntrinsicCostAttributes Attrs(IID, RetTy, Args[0]->getType(), FMF, I, 1);
return getTypeBasedIntrinsicInstrCost(Attrs, CostKind);
} |
||
|
||
IntrinsicCostAttributes NewICA(*FID, ICA.getReturnType(), NewTys, | ||
ICA.getFlags()); | ||
IntrinsicCostAttributes NewICA(*FID, ICA.getReturnType(), NewArgs, | ||
NewTys, ICA.getFlags()); | ||
return thisT()->getIntrinsicInstrCost(NewICA, CostKind); | ||
} | ||
} | ||
|
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 very basic question: why does the arg-based cost differ from the type-based cost for vp.is.fpclass? What information is present in the arguments over and above their types? 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. IIUC the type based costing is needed for places like the LoopVectorizer where we need to cost things that don't yet have The value based costing is used by other transforms that actually have a And for some types of intrinsics and instructions, knowing the value of an operand can actually make the cost more accurate, e.g. if you know the index of a insertelement or the mask of a shufflevector, it can be much cheaper. |
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.
Why do we need isTypeBasedOnly? If it is type-based only, what's the harm in dropping front/back the args?
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.
If it's typeBasedOnly then the args will be empty so I think dropping the front and back will trap