Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
6 changes: 2 additions & 4 deletions llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3069,11 +3069,9 @@ void InnerLoopVectorizer::fixNonInductionPHIs(VPTransformState &State) {
PHINode *NewPhi = cast<PHINode>(State.get(VPPhi));
// Make sure the builder has a valid insert point.
Builder.SetInsertPoint(NewPhi);
for (unsigned Idx = 0; Idx < VPPhi->getNumOperands(); ++Idx) {
VPValue *Inc = VPPhi->getIncomingValue(Idx);
VPBasicBlock *VPBB = VPPhi->getIncomingBlock(Idx);

for (const auto &[Inc, VPBB] : VPPhi->incoming_values_and_blocks())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Perhaps better to rename Inc to Idx? It sounds like a short form of increment or incoming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a short form of incoming value, while Idx would refer to an Index?

NewPhi->addIncoming(State.get(Inc), State.CFG.VPBB2IRBB[VPBB]);
}
}
}
}
Expand Down
65 changes: 58 additions & 7 deletions llvm/lib/Transforms/Vectorize/VPlan.h
Original file line number Diff line number Diff line change
Expand Up @@ -1085,6 +1085,62 @@ class VPIRInstruction : public VPRecipeBase {
void extractLastLaneOfOperand(VPBuilder &Builder);
};

/// Helper type to provide functions to access incoming values and blocks for
/// phi-like recipes. RecipeTy must be a sub-class of VPRecipeBase.
template <typename RecipeTy> class VPPhiAccessors {
/// Return a VPRecipeBase* to the current object.
const VPRecipeBase *getAsRecipe() const {
return static_cast<const RecipeTy *>(this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the templating really needed, can getAsRecipe() simply down cast this to VPRecipeBase?

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 it seems so, problem is that w/o the template there's no inheritance relation here, and static_casts are rejected (Curiously Recurring Template Pattern). For some reason, we need to cast exactly to the type.

But the template parameter may cause problems in the future, so I updated getAsRecipe to be pure virtual to be implemented by the derived classes. The single implementation could also be used for other trait classes in the future. WDYT?

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 use case enabled by getting rid of the template argument is supporting dyn_cast from VPRecipeBase -> VPPhiAccessors, as used in #124838 ( https://github.com/llvm/llvm-project/pull/124838/files#diff-a69094b5fcfce6b2bf9e957e2ac7011e5492e81c885129506c90874375e621fbR210) by implementing CastInfo<VPPhiAccessors, const VPRecipeBase *>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, would having getAsRecipe() do a dyn_cast to VPRecipeBase be ok? Possibly followed by an assert...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we could dyn_cast from VPPhiAccessors to VPRecipeBase because there's no directy relationship between them (and the this pointer for the VPPhiAccessors (sub-)object may not be the same as the VPRecipeBase (base-) object. The other way works, because we can down-cast to the concrete types inheriting from VPPhiAccessors).

}

public:
/// Returns the \p I th incoming VPValue.
VPValue *getIncomingValue(unsigned I) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps rename to Idx to be consistent with getIncomingBlock?

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, thanks

return getAsRecipe()->getOperand(I);
}

/// Returns an interator range over the incoming values
VPUser::const_operand_range incoming_values() const {
return getAsRecipe()->operands();
}

/// Returns the \p I th incoming block.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I this should be Returns the incoming block for \p Idx. since the variable name is Idx

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, thanks

const VPBasicBlock *getIncomingBlock(unsigned Idx) const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Inline as well? Good to see its implementation next to that of getIncomingValue() and getNumIncoming().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs access to VPBasicBlock's definition, which isn't available here; could be resolved by moving VPBlock* definitions to a separate header possibly

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, probably better done separately, if at all.


using const_incoming_block_iterator =
mapped_iterator<detail::index_iterator,
std::function<const VPBasicBlock *(size_t)>>;
using const_incoming_blocks_range =
iterator_range<const_incoming_block_iterator>;

const_incoming_block_iterator incoming_block_begin() const {
return const_incoming_block_iterator(
detail::index_iterator(0),
[this](size_t Idx) { return getIncomingBlock(Idx); });
}
const_incoming_block_iterator incoming_block_end() const {
return const_incoming_block_iterator(
detail::index_iterator(getAsRecipe()->getVPDefID() ==
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you not use some sort of isa<VPWiden...> class check instead of looking at the VPDefID?

Just out of curiosity why doesn't getNumOperands return the correct answer for classes of type VPWidenIntOrFpInductionSC? Wouldn't it be better to fix getNumOperands to return the right answer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity why doesn't getNumOperands return the correct answer for classes of type VPWidenIntOrFpInductionSC?

The operands for VPWidenIntOrFpInductionRecipe aren't the incoming values but are used for constructing them, and they don't match 1-to-1. So I'm not actually sure if VPWidenIntOrFpInductionRecipe should inherit from this as a result?

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 change isn't needed yet, I removed it for now, thanks

VPDef::VPWidenIntOrFpInductionSC
? 2
: getAsRecipe()->getNumOperands()),
[this](size_t Idx) { return getIncomingBlock(Idx); });
}

/// Returns an iterator range over the incoming blocks.
const_incoming_blocks_range incoming_blocks() const {
return make_range(incoming_block_begin(), incoming_block_end());
}

/// Returns an iterator range over pairs of incoming values and corrsponding
/// incoming blocks.
detail::zippy<llvm::detail::zip_shortest, VPUser::const_operand_range,
const_incoming_blocks_range>
incoming_values_and_blocks() const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suffice to keep this method public, as getIncomingValuesAndBlocks(), complementing getIncomingValue(Idx) and getIncomingBlock(Idx)? Trying to reduce non_/camelCase inconsistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now lets just add getIncomingValue and getIncomingBlock

return zip(incoming_values(), incoming_blocks());
}
};

/// VPWidenRecipe is a recipe for producing a widened instruction using the
/// opcode and operands of the recipe. This recipe covers most of the
/// traditional vectorization cases where each recipe transforms into a
Expand Down Expand Up @@ -1944,7 +2000,8 @@ class VPScalarPHIRecipe : public VPHeaderPHIRecipe {
/// recipe is placed in an entry block to a (non-replicate) region, it must have
/// exactly 2 incoming values, the first from the predecessor of the region and
/// the second from the exiting block of the region.
class VPWidenPHIRecipe : public VPSingleDefRecipe {
class VPWidenPHIRecipe : public VPSingleDefRecipe,
public VPPhiAccessors<VPWidenPHIRecipe> {
public:
/// Create a new VPWidenPHIRecipe for \p Phi with start value \p Start and
/// debug location \p DL.
Expand All @@ -1970,12 +2027,6 @@ class VPWidenPHIRecipe : public VPSingleDefRecipe {
void print(raw_ostream &O, const Twine &Indent,
VPSlotTracker &SlotTracker) const override;
#endif

/// Returns the \p I th incoming VPBasicBlock.
VPBasicBlock *getIncomingBlock(unsigned I);

/// Returns the \p I th incoming VPValue.
VPValue *getIncomingValue(unsigned I) { return getOperand(I); }
};

/// A recipe for handling first-order recurrence phis. The start value is the
Expand Down
46 changes: 27 additions & 19 deletions llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,29 @@ void VPIRInstruction::print(raw_ostream &O, const Twine &Indent,
}
#endif

static const VPBasicBlock *getIncomingBlockForRecipe(const VPRecipeBase *R,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth using the same Idx variable name as getIncomingBlock for consistency? Or vice-versa - rename all instances of Idx to I?

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 to use Idx consistently.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be getCFGPredecessors(VPBasicBlock) or VPBasicBlock:: getCFGPredecessors(), as iterators and/or single element by index? Complementing getHierarchicalPredecessors() and getPredecessors().

OTOH, if used only by VPPhiAccessors::getIncomingBlock(Idx), better inline it there. But by templating VPPhiAccessors, this would need to be replicated per instance, as in VPPhiAccessors<VPWidenPHIRecipe>::getIncomingBlock(Idx)?

This provides the CFG predecessor basic-blocks of a given block (rather than recipe), which could be in CFG mode (in which case they are held explicitly, can cast them from block to basic-block) or HCFG mode (in which case region header blocks need to collect their region's predecessor('s exiting) basic-block and exiting basic-block.

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 had a similar thought, but wasn't sure what a good name would be. Added as getCFGPredecessor(Idx) to start with. Could add an iterator version separately when other iterators are added

unsigned I) {
const VPBasicBlock *Parent = R->getParent();
const VPBlockBase *Pred = nullptr;
if (Parent->getNumPredecessors() > 0) {
Pred = Parent->getPredecessors()[I];
} else {
auto *Region = Parent->getParent();
assert(Region && !Region->isReplicator() && Region->getEntry() == Parent &&
"must be in the entry block of a non-replicate region");
assert(
I < 2 &&
(R->getNumOperands() == 2 || isa<VPWidenIntOrFpInductionRecipe>(R)) &&
"when placed in an entry block, only 2 incoming blocks are available");

// I == 0 selects the predecessor of the region, I == 1 selects the region
// itself whose exiting block feeds the phi across the backedge.
Pred = I == 0 ? Region->getSinglePredecessor() : Region;
}

return Pred->getExitingBasicBlock();
}

void VPWidenCallRecipe::execute(VPTransformState &State) {
assert(State.VF.isVector() && "not widening");
State.setDebugLocFrom(getDebugLoc());
Expand Down Expand Up @@ -3580,25 +3603,10 @@ void VPReductionPHIRecipe::print(raw_ostream &O, const Twine &Indent,
}
#endif

VPBasicBlock *VPWidenPHIRecipe::getIncomingBlock(unsigned I) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better keep delegating the implementation of getIncomingBlock(Idx) to recipes that inherit from VPPhiAccessors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now providing a specialized definition of VPPhiAccessors<VPWidenPHIRecipe>::getIncomingBlock, which is needed to instantiate the template for the type and use the generic getIncomingBlockForRecipe

VPBasicBlock *Parent = getParent();
VPBlockBase *Pred = nullptr;
if (Parent->getNumPredecessors() > 0) {
Pred = Parent->getPredecessors()[I];
} else {
auto *Region = Parent->getParent();
assert(Region && !Region->isReplicator() && Region->getEntry() == Parent &&
"must be in the entry block of a non-replicate region");
assert(
I < 2 && getNumOperands() == 2 &&
"when placed in an entry block, only 2 incoming blocks are available");

// I == 0 selects the predecessor of the region, I == 1 selects the region
// itself whose exiting block feeds the phi across the backedge.
Pred = I == 0 ? Region->getSinglePredecessor() : Region;
}

return Pred->getExitingBasicBlock();
template <>
const VPBasicBlock *
VPPhiAccessors<VPWidenPHIRecipe>::getIncomingBlock(unsigned Idx) const {
return getIncomingBlockForRecipe(getAsRecipe(), Idx);
}

void VPWidenPHIRecipe::execute(VPTransformState &State) {
Expand Down