Skip to content

[VPlan] Support live-ins without underlying IR in type analysis. #80723

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Feb 21, 2024
Merged

[VPlan] Support live-ins without underlying IR in type analysis. #80723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
15b2c7c
[VPlan] Support live-ins without underlying IR in type analysis.
fhahn Feb 5, 2024
7d88cd3
Merge branch 'main' into vplan-type-analysis-live-ins
fhahn Feb 21, 2024
c49ca00
!fixup store canonica iv type in VPTypeAnalysis.
fhahn Feb 21, 2024
e5f9fec
Merge branch 'main' into vplan-type-analysis-live-ins
fhahn Feb 21, 2024
7e9b7a1
!fixup address latest comments, thanks!
fhahn Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions llvm/lib/Transforms/Vectorize/VPlan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,14 @@ VPBasicBlock::iterator VPBasicBlock::getFirstNonPhi() {
return It;
}

VPTransformState::VPTransformState(ElementCount VF, unsigned UF, LoopInfo *LI,
DominatorTree *DT, IRBuilderBase &Builder,
InnerLoopVectorizer *ILV, VPlan *Plan,
LLVMContext &Ctx)
: VF(VF), UF(UF), LI(LI), DT(DT), Builder(Builder), ILV(ILV), Plan(Plan),
LVer(nullptr),
TypeAnalysis(Plan->getCanonicalIV()->getScalarType(), Ctx) {}

Value *VPTransformState::get(VPValue *Def, const VPIteration &Instance) {
if (Def->isLiveIn())
return Def->getLiveInIRValue();
Expand Down
4 changes: 1 addition & 3 deletions llvm/lib/Transforms/Vectorize/VPlan.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,7 @@ struct VPIteration {
struct VPTransformState {
VPTransformState(ElementCount VF, unsigned UF, LoopInfo *LI,
DominatorTree *DT, IRBuilderBase &Builder,
InnerLoopVectorizer *ILV, VPlan *Plan, LLVMContext &Ctx)
: VF(VF), UF(UF), LI(LI), DT(DT), Builder(Builder), ILV(ILV), Plan(Plan),
LVer(nullptr), TypeAnalysis(Ctx) {}
InnerLoopVectorizer *ILV, VPlan *Plan, LLVMContext &Ctx);

/// The chosen Vectorization and Unroll Factors of the loop being vectorized.
ElementCount VF;
Expand Down
16 changes: 8 additions & 8 deletions llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,7 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPInstruction *R) {
CachedTypes[OtherV] = ResTy;
return ResTy;
}
case Instruction::ICmp: {
// TODO: Check if types for both operands agree. This also requires
// type-inference for the vector-trip-count, which is missing at the moment.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should have probably referred to backedge-taken-count, used by an ICmp to form the header mask, rather than vector-trip-count.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

Type *ResTy = inferScalarType(R->getOperand(0));
return ResTy;
}
case Instruction::ICmp:
case VPInstruction::FirstOrderRecurrenceSplice: {
Type *ResTy = inferScalarType(R->getOperand(0));
VPValue *OtherV = R->getOperand(1);
Expand Down Expand Up @@ -207,8 +202,13 @@ Type *VPTypeAnalysis::inferScalarType(const VPValue *V) {
if (Type *CachedTy = CachedTypes.lookup(V))
return CachedTy;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to caching the pre-computed type for all live-ins-w/o-underlying-value, as raised above, may be to change below into:

  if (V->isLiveIn()) {
    if (auto *IRValue = V->getLiveInIRValue())
      return IRValue->getType();
    return CanonicalType;
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as suggested, thanks!


if (V->isLiveIn())
return V->getLiveInIRValue()->getType();
if (V->isLiveIn()) {
if (auto *IRValue = V->getLiveInIRValue())
return IRValue->getType();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: worth a comment to explain who's expected to use CanonicalIVTy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment ,thanks!

// All VPValues without any underlying IR value (like the vector trip count
// or the backedge-taken count) have the same type as the canonical IV.
return CanonicalIVTy;
}

Type *ResultTy =
TypeSwitch<const VPRecipeBase *, Type *>(V->getDefiningRecipe())
Expand Down
7 changes: 6 additions & 1 deletion llvm/lib/Transforms/Vectorize/VPlanAnalysis.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ class Type;
/// of the previously inferred types.
class VPTypeAnalysis {
DenseMap<const VPValue *, Type *> CachedTypes;
/// Type of the canonical induction variable. Used for all VPValues without
/// any underlying IR value (like the vector trip count or the backedge-taken
/// count).
Type *CanonicalIVTy;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: worth a comment what this is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added ,thanks!

LLVMContext &Ctx;

Type *inferScalarTypeForRecipe(const VPBlendRecipe *R);
Expand All @@ -47,7 +51,8 @@ class VPTypeAnalysis {
Type *inferScalarTypeForRecipe(const VPReplicateRecipe *R);

public:
VPTypeAnalysis(LLVMContext &Ctx) : Ctx(Ctx) {}
VPTypeAnalysis(Type *CanonicalIVTy, LLVMContext &Ctx)
: CanonicalIVTy(CanonicalIVTy), Ctx(Ctx) {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (unrelated to this patch): does VPTypeAnalysis really need Ctx?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could retrieve the context from a type separately when needed. Can do separately if preferred?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any current use of Ctx? Sure, if this is indeed dead, should be eliminated separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current uses are to create i1 & void types:

https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp#L63
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp#L163
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp#L189


/// Infer the type of \p V. Returns the scalar type of \p V.
Type *inferScalarType(const VPValue *V);
Expand Down
11 changes: 7 additions & 4 deletions llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,8 @@ static VPValue *createScalarIVSteps(VPlan &Plan, const InductionDescriptor &ID,
}

// Truncate base induction if needed.
VPTypeAnalysis TypeInfo(SE.getContext());
VPTypeAnalysis TypeInfo(Plan.getCanonicalIV()->getScalarType(),
SE.getContext());
Type *ResultTy = TypeInfo.inferScalarType(BaseIV);
if (TruncI) {
Type *TruncTy = TruncI->getType();
Expand Down Expand Up @@ -897,7 +898,9 @@ static void simplifyRecipe(VPRecipeBase &R, VPTypeAnalysis &TypeInfo) {
#ifndef NDEBUG
// Verify that the cached type info is for both A and its users is still
// accurate by comparing it to freshly computed types.
VPTypeAnalysis TypeInfo2(TypeInfo.getContext());
VPTypeAnalysis TypeInfo2(
R.getParent()->getPlan()->getCanonicalIV()->getScalarType(),
TypeInfo.getContext());
assert(TypeInfo.inferScalarType(A) == TypeInfo2.inferScalarType(A));
for (VPUser *U : A->users()) {
auto *R = dyn_cast<VPRecipeBase>(U);
Expand All @@ -918,7 +921,7 @@ static void simplifyRecipe(VPRecipeBase &R, VPTypeAnalysis &TypeInfo) {
static void simplifyRecipes(VPlan &Plan, LLVMContext &Ctx) {
ReversePostOrderTraversal<VPBlockDeepTraversalWrapper<VPBlockBase *>> RPOT(
Plan.getEntry());
VPTypeAnalysis TypeInfo(Ctx);
VPTypeAnalysis TypeInfo(Plan.getCanonicalIV()->getScalarType(), Ctx);
for (VPBasicBlock *VPBB : VPBlockUtils::blocksOnly<VPBasicBlock>(RPOT)) {
for (VPRecipeBase &R : make_early_inc_range(*VPBB)) {
simplifyRecipe(R, TypeInfo);
Expand All @@ -939,7 +942,7 @@ void VPlanTransforms::truncateToMinimalBitwidths(
// other uses have different types for their operands, making them invalidly
// typed.
DenseMap<VPValue *, VPWidenCastRecipe *> ProcessedTruncs;
VPTypeAnalysis TypeInfo(Ctx);
VPTypeAnalysis TypeInfo(Plan.getCanonicalIV()->getScalarType(), Ctx);
VPBasicBlock *PH = Plan.getEntry();
for (VPBasicBlock *VPBB : VPBlockUtils::blocksOnly<VPBasicBlock>(
vp_depth_first_deep(Plan.getVectorLoopRegion()))) {
Expand Down