-
Notifications
You must be signed in to change notification settings - Fork 14k
[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 all 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 *ResultTy = TypeInfo.inferScalarType(BaseIV); | ||
if (TruncI) { | ||
Type *TruncTy = TruncI->getType(); | ||
assert(ResultTy->getScalarSizeInBits() > TruncTy->getScalarSizeInBits() && | ||
"Not truncating."); | ||
assert(ResultTy->isIntegerTy() && "Truncation requires an integer type"); | ||
BaseIV = new VPScalarCastRecipe(Instruction::Trunc, BaseIV, TruncTy); | ||
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. Ah, ResultTy should be reset to TruncTy here, so that Step will be truncated to the updated type of BaseIV, rather than its original type. (Or reset right after the asserts, so that both truncations use ResultTy.) Wonder about test coverage if all pass? 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. Argh, sent the update too early (or too late in the day)! Should be fixed now., there were a number of test failures. |
||
HeaderVPBB->insert(BaseIV, IP); | ||
ResultTy = TruncTy; | ||
} | ||
|
||
// Truncate step if needed. | ||
Type *StepTy = TypeInfo.inferScalarType(Step); | ||
if (ResultTy != StepTy) { | ||
assert(StepTy->getScalarSizeInBits() > ResultTy->getScalarSizeInBits() && | ||
"Not truncating."); | ||
assert(StepTy->isIntegerTy() && "Truncation requires an integer type"); | ||
Step = new VPScalarCastRecipe(Instruction::Trunc, Step, ResultTy); | ||
auto *VecPreheader = | ||
cast<VPBasicBlock>(HeaderVPBB->getSingleHierarchicalPredecessor()); | ||
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.