-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[VPlan] Add new VPScalarCastRecipe, use for IV & step trunc. #78113
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 10 commits
36b085f
f5f961e
6b3e52e
9331a45
9988d78
fe5ec9e
5500bdb
18cd7d4
f1f1eff
d382232
6efa577
e756798
3c31111
dc7990d
adc5441
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 |
---|---|---|
|
@@ -230,7 +230,13 @@ Type *VPTypeAnalysis::inferScalarType(const VPValue *V) { | |
return V->getUnderlyingValue()->getType(); | ||
}) | ||
.Case<VPWidenCastRecipe>( | ||
[](const VPWidenCastRecipe *R) { return R->getResultType(); }); | ||
[](const VPWidenCastRecipe *R) { return R->getResultType(); }) | ||
.Case<VPScalarCastRecipe>( | ||
[](const VPScalarCastRecipe *R) { return R->getResultType(); }) | ||
.Case<VPExpandSCEVRecipe>([](const VPExpandSCEVRecipe *R) { | ||
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. Adding Expand SCEV case to inferScalarType is independent of this patch? 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. It is needed to infer the type of the step in the new code in VPlanTransforms.cpp ( 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. So now that VPDerivedIVRecipe simply mirrors the type of its first start-value operand, should it join VPCanonicalIVPHIRecipe et al.'s or VPPredInstPHIRecipe et al.'s cases above and remove its getScalarType() method? Possibly as a follow-up. Should VPWidenIntOrFpInductionRecipe's trunc also utilize VPScalarCastRecipe, and retire its getScalarType(), possibly as a follow-up? 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. VPDerivedIVRecipe getScalarType would be good to remove, will do as follow-up.
|
||
return R->getSCEV()->getType(); | ||
}); | ||
|
||
assert(ResultTy && "could not infer type for the given VPValue"); | ||
CachedTypes[V] = ResultTy; | ||
return ResultTy; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,6 +117,7 @@ bool VPRecipeBase::mayHaveSideEffects() const { | |
switch (getVPDefID()) { | ||
case VPDerivedIVSC: | ||
case VPPredInstPHISC: | ||
case VPScalarCastSC: | ||
return false; | ||
case VPInstructionSC: | ||
switch (cast<VPInstruction>(this)->getOpcode()) { | ||
|
@@ -1096,9 +1097,6 @@ void VPDerivedIVRecipe::print(raw_ostream &O, const Twine &Indent, | |
getCanonicalIV()->printAsOperand(O, SlotTracker); | ||
O << " * "; | ||
getStepValue()->printAsOperand(O, SlotTracker); | ||
|
||
if (TruncResultTy) | ||
O << " (truncated to " << *TruncResultTy << ")"; | ||
} | ||
#endif | ||
|
||
|
@@ -1117,13 +1115,7 @@ void VPScalarIVStepsRecipe::execute(VPTransformState &State) { | |
|
||
// Ensure step has the same type as that of scalar IV. | ||
Type *BaseIVTy = BaseIV->getType()->getScalarType(); | ||
if (BaseIVTy != Step->getType()) { | ||
// TODO: Also use VPDerivedIVRecipe when only the step needs truncating, to | ||
// avoid separate truncate here. | ||
assert(Step->getType()->isIntegerTy() && | ||
"Truncation requires an integer step"); | ||
Step = State.Builder.CreateTrunc(Step, BaseIVTy); | ||
} | ||
assert(BaseIVTy == Step->getType() && "Types of BaseIV and Step must match!"); | ||
|
||
// We build scalar steps for both integer and floating-point induction | ||
// variables. Here, we determine the kind of arithmetic we will perform. | ||
|
@@ -1469,6 +1461,58 @@ void VPReplicateRecipe::print(raw_ostream &O, const Twine &Indent, | |
} | ||
#endif | ||
|
||
/// Checks if \p C is uniform across all VFs and UFs. It is considered as such | ||
/// if it is either defined outside the vector region or its operand is known to | ||
/// be uniform across all VFs and UFs (e.g. VPDerivedIV or VPCanonicalIVPHI). | ||
/// TODO: Uniformity should be associated with a VPValue and there should be a | ||
/// generic way to check. | ||
static bool isUniformAcrossVFsAndUFs(VPScalarCastRecipe *C) { | ||
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. Worth documenting, potentially leaving behind a note that uniformity should essentially be associated with VPValues. 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. Added comment + TODO, thanks! |
||
return C->isDefinedOutsideVectorRegions() || | ||
isa<VPDerivedIVRecipe>(C->getOperand(0)) || | ||
isa<VPCanonicalIVPHIRecipe>(C->getOperand(0)); | ||
} | ||
|
||
Value *VPScalarCastRecipe ::generate(VPTransformState &State, unsigned Part) { | ||
assert(vputils::onlyFirstLaneUsed(this) && | ||
"Codegen only implemented for first lane."); | ||
switch (Opcode) { | ||
case Instruction::SExt: | ||
case Instruction::ZExt: | ||
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. Only Trunc is currently being used, still. 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. Yep, could remove if preferred, but 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. Would be good to either remove or make a note that SExt and ZExt are currently unused/dead cases. 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. Added a note, thanks! |
||
case Instruction::Trunc: { | ||
// Note: SExt/ZExt not used yet. | ||
Value *Op = State.get(getOperand(0), VPIteration(Part, 0)); | ||
return State.Builder.CreateCast(Instruction::CastOps(Opcode), Op, ResultTy); | ||
} | ||
default: | ||
llvm_unreachable("opcode not implemented yet"); | ||
} | ||
} | ||
|
||
void VPScalarCastRecipe ::execute(VPTransformState &State) { | ||
bool IsUniformAcrossVFsAndUFs = isUniformAcrossVFsAndUFs(this); | ||
for (unsigned Part = 0; Part != State.UF; ++Part) { | ||
Value *Res; | ||
// Only generate a single instance, if the recipe is uniform across UFs and | ||
// VFs. | ||
if (Part > 0 && IsUniformAcrossVFsAndUFs) | ||
Res = State.get(this, VPIteration(0, 0)); | ||
else | ||
Res = generate(State, Part); | ||
State.set(this, Res, VPIteration(Part, 0)); | ||
} | ||
} | ||
|
||
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) | ||
void VPScalarCastRecipe ::print(raw_ostream &O, const Twine &Indent, | ||
VPSlotTracker &SlotTracker) const { | ||
O << Indent << "SCALAR-CAST "; | ||
printAsOperand(O, SlotTracker); | ||
O << " = " << Instruction::getOpcodeName(Opcode) << " "; | ||
printOperands(O, SlotTracker); | ||
O << " to " << *ResultTy; | ||
} | ||
#endif | ||
|
||
void VPBranchOnMaskRecipe::execute(VPTransformState &State) { | ||
assert(State.Instance && "Branch on Mask works only on single instance."); | ||
|
||
|
@@ -1587,10 +1631,10 @@ void VPCanonicalIVPHIRecipe::print(raw_ostream &O, const Twine &Indent, | |
#endif | ||
|
||
bool VPCanonicalIVPHIRecipe::isCanonical( | ||
InductionDescriptor::InductionKind Kind, VPValue *Start, VPValue *Step, | ||
Type *Ty) const { | ||
// The types must match and it must be an integer induction. | ||
if (Ty != getScalarType() || Kind != InductionDescriptor::IK_IntInduction) | ||
InductionDescriptor::InductionKind Kind, VPValue *Start, | ||
VPValue *Step) const { | ||
// Must be an integer induction. | ||
if (Kind != InductionDescriptor::IK_IntInduction) | ||
return false; | ||
// Start must match the start value of this canonical induction. | ||
if (Start != getStartValue()) | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -491,17 +491,39 @@ void VPlanTransforms::removeDeadRecipes(VPlan &Plan) { | |||||||||||||||||
|
||||||||||||||||||
static VPValue *createScalarIVSteps(VPlan &Plan, const InductionDescriptor &ID, | ||||||||||||||||||
ScalarEvolution &SE, Instruction *TruncI, | ||||||||||||||||||
Type *IVTy, VPValue *StartV, | ||||||||||||||||||
VPValue *Step) { | ||||||||||||||||||
VPValue *StartV, VPValue *Step) { | ||||||||||||||||||
VPBasicBlock *HeaderVPBB = Plan.getVectorLoopRegion()->getEntryBasicBlock(); | ||||||||||||||||||
auto IP = HeaderVPBB->getFirstNonPhi(); | ||||||||||||||||||
VPCanonicalIVPHIRecipe *CanonicalIV = Plan.getCanonicalIV(); | ||||||||||||||||||
Type *TruncTy = TruncI ? TruncI->getType() : IVTy; | ||||||||||||||||||
VPValue *BaseIV = CanonicalIV; | ||||||||||||||||||
if (!CanonicalIV->isCanonical(ID.getKind(), StartV, Step, TruncTy)) { | ||||||||||||||||||
BaseIV = new VPDerivedIVRecipe(ID, StartV, CanonicalIV, Step, | ||||||||||||||||||
TruncI ? TruncI->getType() : nullptr); | ||||||||||||||||||
HeaderVPBB->insert(BaseIV->getDefiningRecipe(), IP); | ||||||||||||||||||
VPSingleDefRecipe *BaseIV = CanonicalIV; | ||||||||||||||||||
if (!CanonicalIV->isCanonical(ID.getKind(), StartV, Step)) { | ||||||||||||||||||
BaseIV = new VPDerivedIVRecipe(ID, StartV, CanonicalIV, Step); | ||||||||||||||||||
HeaderVPBB->insert(BaseIV, IP); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// Truncate base induction if needed. | ||||||||||||||||||
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.
both BaseIV and Step are subject to redefinition and truncation, perhaps aim to define the type of the result which they may truncate to. 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, thanks! (missed originally) |
||||||||||||||||||
VPTypeAnalysis TypeInfo(SE.getContext()); | ||||||||||||||||||
Type *StepTy = TypeInfo.inferScalarType(Step); | ||||||||||||||||||
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.
Suggested change
Suffice to define StepTy later, when dealing with the truncation of Step. Here we're dealing with truncating BaseIV, and may be good to define its current/original type instead (using "BaseIVTy" may be confusing or deserves updating due to changing BaseIV and its type). 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. Updated, thanks! |
||||||||||||||||||
if (TruncI) { | ||||||||||||||||||
Type *TruncTy = TruncI->getType(); | ||||||||||||||||||
assert(TypeInfo.inferScalarType(BaseIV)->getScalarSizeInBits() > | ||||||||||||||||||
TruncTy->getScalarSizeInBits() && | ||||||||||||||||||
StepTy->isIntegerTy() && "Truncation requires an integer step"); | ||||||||||||||||||
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.
Suggested change
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. Updated and also separate into 2 separate asserts. 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.
Suggested change
this should assert that the truncation of BaseIV is valid, independent of Step. As in the proposal above, which uses ResultTy. 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. Updated, thanks! |
||||||||||||||||||
auto *T = new VPScalarCastRecipe(Instruction::Trunc, BaseIV, TruncTy); | ||||||||||||||||||
HeaderVPBB->insert(T, IP); | ||||||||||||||||||
BaseIV = T; | ||||||||||||||||||
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.
Suggested change
? 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. There's no real need in 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. Adjusted, thanks! |
||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// Truncate step if needed. | ||||||||||||||||||
Type *ResultTy = TypeInfo.inferScalarType(BaseIV); | ||||||||||||||||||
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. Define StepTy here? 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. Moved, thanks! |
||||||||||||||||||
if (ResultTy != StepTy) { | ||||||||||||||||||
assert(StepTy->getScalarSizeInBits() > ResultTy->getScalarSizeInBits() && | ||||||||||||||||||
StepTy->isIntegerTy() && "Truncation requires an integer step"); | ||||||||||||||||||
|
||||||||||||||||||
Step = new VPScalarCastRecipe(Instruction::Trunc, Step, ResultTy); | ||||||||||||||||||
auto *VecPreheader = | ||||||||||||||||||
cast<VPBasicBlock>(Plan.getVectorLoopRegion()->getSinglePredecessor()); | ||||||||||||||||||
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.
Suggested change
? 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, thanks! |
||||||||||||||||||
VecPreheader->appendRecipe(Step->getDefiningRecipe()); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
VPScalarIVStepsRecipe *Steps = new VPScalarIVStepsRecipe(ID, BaseIV, Step); | ||||||||||||||||||
|
@@ -523,9 +545,9 @@ void VPlanTransforms::optimizeInductions(VPlan &Plan, ScalarEvolution &SE) { | |||||||||||||||||
continue; | ||||||||||||||||||
|
||||||||||||||||||
const InductionDescriptor &ID = WideIV->getInductionDescriptor(); | ||||||||||||||||||
VPValue *Steps = createScalarIVSteps( | ||||||||||||||||||
Plan, ID, SE, WideIV->getTruncInst(), WideIV->getPHINode()->getType(), | ||||||||||||||||||
WideIV->getStartValue(), WideIV->getStepValue()); | ||||||||||||||||||
VPValue *Steps = | ||||||||||||||||||
createScalarIVSteps(Plan, ID, SE, WideIV->getTruncInst(), | ||||||||||||||||||
WideIV->getStartValue(), WideIV->getStepValue()); | ||||||||||||||||||
|
||||||||||||||||||
// Update scalar users of IV to use Step instead. | ||||||||||||||||||
if (!HasOnlyVectorVFs) | ||||||||||||||||||
|
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.
nit (independent of this patch): would be good to keep in order.
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.
Yep, will do separately.