[VPlan] Add VPPhiAccessors to provide interface for phi recipes (NFC) #129388

fhahn · 2025-03-01T15:57:49Z

Add a VPPhiAccessors class to provide interfaces to access incoming values and blocks, with corresponding iterators.

The first user is VPWidenPhiRecipe, with the other phi-like recipes following soon.

This will also be used to verify def-use chains where users are phi-like recipes, simplifying #124838.

Add a VPPhiAccessors class to provide interfaces to access incoming values and blocks, with corresponding iterators. The first user is VPWidenPhiRecipe, with the other phi-like recipes following soon. This will also be used to verify def-use chains where users are phi-like recipes, simplifying llvm#124838.

llvmbot · 2025-03-01T15:58:22Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-vectorizers

Author: Florian Hahn (fhahn)

Changes

Add a VPPhiAccessors class to provide interfaces to access incoming values and blocks, with corresponding iterators.

The first user is VPWidenPhiRecipe, with the other phi-like recipes following soon.

This will also be used to verify def-use chains where users are phi-like recipes, simplifying #124838.

Full diff: https://github.com/llvm/llvm-project/pull/129388.diff

3 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+2-4)
(modified) llvm/lib/Transforms/Vectorize/VPlan.h (+58-7)
(modified) llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp (+27-19)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index e2612698b6b0f..98ff1a4cdc449 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -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())
         NewPhi->addIncoming(State.get(Inc), State.CFG.VPBB2IRBB[VPBB]);
-      }
     }
   }
 }
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 70e684826ed2d..4d5f2014a40dc 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -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);
+  }
+
+public:
+  /// Returns the \p I th incoming VPValue.
+  VPValue *getIncomingValue(unsigned I) const {
+    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.
+  const VPBasicBlock *getIncomingBlock(unsigned Idx) const;
+
+  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() ==
+                                       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 {
+    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
@@ -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.
@@ -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
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index e9f50e88867b2..efac05785203c 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -1031,6 +1031,29 @@ void VPIRInstruction::print(raw_ostream &O, const Twine &Indent,
 }
 #endif
 
+static const VPBasicBlock *getIncomingBlockForRecipe(const VPRecipeBase *R,
+                                                     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());
@@ -3580,25 +3603,10 @@ void VPReductionPHIRecipe::print(raw_ostream &O, const Twine &Indent,
 }
 #endif
 
-VPBasicBlock *VPWidenPHIRecipe::getIncomingBlock(unsigned I) {
-  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) {

lukel97

Can VPHeaderPHIRecipe and its subclasses inherit from this too? Where the incoming values are { getStartValue(), getBackedgeValue() } and the incoming blocks are { loopPreheader, exitingBlock } (or {region->getSinglePredecessor, region }?)

lukel97

LGTM, makes sense. I just left some small questions

llvm/lib/Transforms/Vectorize/VPlan.h

david-arm · 2025-03-03T10:14:50Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

-        VPValue *Inc = VPPhi->getIncomingValue(Idx);
-        VPBasicBlock *VPBB = VPPhi->getIncomingBlock(Idx);
+
+      for (const auto &[Inc, VPBB] : VPPhi->incoming_values_and_blocks())


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

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

david-arm · 2025-03-03T10:15:51Z

llvm/lib/Transforms/Vectorize/VPlan.h

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


Perhaps rename to Idx to be consistent with getIncomingBlock?

Updated, thanks

david-arm · 2025-03-03T10:16:52Z

llvm/lib/Transforms/Vectorize/VPlan.h

+    return getAsRecipe()->operands();
+  }
+
+  /// Returns the \p I th incoming block.


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

Updated, thanks

david-arm · 2025-03-03T10:19:36Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  }
+  const_incoming_block_iterator incoming_block_end() const {
+    return const_incoming_block_iterator(
+        detail::index_iterator(getAsRecipe()->getVPDefID() ==


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?

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?

The change isn't needed yet, I removed it for now, thanks

david-arm · 2025-03-03T10:24:44Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

@@ -1031,6 +1031,29 @@ void VPIRInstruction::print(raw_ostream &O, const Twine &Indent,
 }
 #endif

+static const VPBasicBlock *getIncomingBlockForRecipe(const VPRecipeBase *R,


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

Updated to use Idx consistently.

Now that all phi nodes manage their incoming blocks through the VPlan-predecessors, there should be no need for having a dedicate recipe, it should be sufficient to allow PHI opcodes in VPInstruction. Follow-ups will also migrate VPWidenPHIRecipe and possibly others, building on top of llvm#129388.

Now that all phi nodes manage their incoming blocks through the VPlan-predecessors, there should be no need for having a dedicate recipe, it should be sufficient to allow PHI opcodes in VPInstruction. Follow-ups will also migrate VPWidenPHIRecipe and possibly others, building on top of #129388. PR: #129767

…129767) Now that all phi nodes manage their incoming blocks through the VPlan-predecessors, there should be no need for having a dedicate recipe, it should be sufficient to allow PHI opcodes in VPInstruction. Follow-ups will also migrate VPWidenPHIRecipe and possibly others, building on top of llvm/llvm-project#129388. PR: llvm/llvm-project#129767

Now that all phi nodes manage their incoming blocks through the VPlan-predecessors, there should be no need for having a dedicate recipe, it should be sufficient to allow PHI opcodes in VPInstruction. Follow-ups will also migrate VPWidenPHIRecipe and possibly others, building on top of llvm#129388. PR: llvm#129767

fhahn

ping :)

llvm/lib/Transforms/Vectorize/VPlan.h

ayalz · 2025-04-28T14:58:57Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

-      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())
        NewPhi->addIncoming(State.get(Inc), State.CFG.VPBB2IRBB[VPBB]);
-      }


Is this return (in simplification?) worth the investment. Would an API of getIncomingValue(Idx), getIncomingBlock(Idx), and possibly getNumIncomings() common to all phi recipes suffice.

There will be additional users when updating more recipes to use the accessors; they are more conveiient, but getIncomingValue and getIncomingBlock would also suffice, at the cost of being a bit less convenient.

How about first wrapping getIncomingValue and getIncomingBlock within VPPhiAccessors, and introduce the additional zipped iterator API as a separate follow-up. That would help clarify the convenience versus investment.

Ah yes, done for now, thanks

ayalz · 2025-04-28T15:07:31Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  VPValue *getIncomingValue(unsigned Idx) const {
+    return getAsRecipe()->getOperand(Idx);
+  }


How about VPPhiAccessors having both getIncomingBlock(Idx) and getIncomingValue(Idx) be pure virtual, and have another interim VPSingleDefPhiRecipe inherit from both VPSingleDefRecipe and VPPhiAccessors take care of implementing getIncomingValue(Idx) for all (singleDef) phi recipes, instead of getAsRecipe() subclass casting. Are all recipes candidate of inheritance singleDef?

All recipes are single defs, but we now unfortunately have some recipes (e.g. VPIRPhi) where the base class VPIRInstruction inherits from VPSingleDefRecipe, but inheriting from VPSinglePhiDefRecipe would not be approriate, hence the trait/mix-in. Down the road, we could also support casting any recipe that supports it to VPPhiAccessors, e.g. for verifying all phi-like nodes that implement the trait.

Alternatively we could manually add definitions of getIncomingBlock and getIncomingValue to all relevant classes?

OK, who are all the relevant classes - expected to inherit directly from VPPhiAccessors in addition to VPWidenPHIRecipe (who could use VPSingleDefPhiRecipe with other potential partners) and VPIRPhi?

If VPIRPhi inherits directly from VPPhiAccessors, could it implement getIncomingBlock based on the direct predecessors of its VPBasicBlock, as it is not used to represent header phi's of HCFG regions? I.e., assert it has direct predecessors.

In any case, good to implement both getIncomingBlock and getIncomingValue by the mix-in, as done here, or neither (and have both defined by all derived classes instead).

we need it for VPWidenPHIRecipe, VPHeaderPHIRecipe, VPIRPhi, VPEVLBasedPhi and VPPhi (scalar phis via VPInstruction, probably via a new specialization).

Define both getIncomingBlock and getIncomingValue there

Another one is VPPredInstPhi

VPWidenPHIRecipe, VPHeaderPHIRecipe (base class of VPEVLBasedPhi), and VPPredInstPhi all inherit from VPSingleDefRecipe. So could inherit from VPSingleDefPhiRecipe instead, which could take care of implementing these pure virtual methods for them.

Made the getAsRecipe pure-virtual to avoid the template argument + static cast. WDYT?

ayalz · 2025-04-28T15:16:30Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

@@ -3694,25 +3720,10 @@ void VPReductionPHIRecipe::print(raw_ostream &O, const Twine &Indent,
 }
 #endif

-VPBasicBlock *VPWidenPHIRecipe::getIncomingBlock(unsigned I) {


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

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

ayalz · 2025-05-01T09:21:36Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+/// recipes placed in entry blocks of loop regions (incoming blocks are the
+/// region's predecessor and the region's exit) and other locations (incoming
+/// blocks are the direct predecessors).
+static const VPBasicBlock *getIncomingBlockForRecipe(const VPRecipeBase *R,


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.

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

ayalz · 2025-05-01T10:07:48Z

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

-      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())
        NewPhi->addIncoming(State.get(Inc), State.CFG.VPBB2IRBB[VPBB]);
-      }


How about first wrapping getIncomingValue and getIncomingBlock within VPPhiAccessors, and introduce the additional zipped iterator API as a separate follow-up. That would help clarify the convenience versus investment.

ayalz · 2025-05-01T10:20:49Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  VPValue *getIncomingValue(unsigned Idx) const {
+    return getAsRecipe()->getOperand(Idx);
+  }


OK, who are all the relevant classes - expected to inherit directly from VPPhiAccessors in addition to VPWidenPHIRecipe (who could use VPSingleDefPhiRecipe with other potential partners) and VPIRPhi?

If VPIRPhi inherits directly from VPPhiAccessors, could it implement getIncomingBlock based on the direct predecessors of its VPBasicBlock, as it is not used to represent header phi's of HCFG regions? I.e., assert it has direct predecessors.

In any case, good to implement both getIncomingBlock and getIncomingValue by the mix-in, as done here, or neither (and have both defined by all derived classes instead).

ayalz · 2025-05-01T10:23:56Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  /// incoming blocks.
+  detail::zippy<llvm::detail::zip_shortest, VPUser::const_operand_range,
+                const_incoming_blocks_range>
+  incoming_values_and_blocks() const {


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

for now lets just add getIncomingValue and getIncomingBlock

ayalz · 2025-05-01T10:28:25Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  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()->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());
+  }


If/when predecessors already have a natural iterator, would building an index_iterator from retrieving each block get folded into the former?

I can check once we bring those iterators back

ayalz

LGTM, thanks!

ayalz · 2025-05-02T07:36:33Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+           "must be in the entry block of a non-replicate region");
+    assert(
+        Idx < 2 && Region->getNumPredecessors() == 1 &&
+        "when placed in an entry block, only 2 incoming blocks are available");


Suggested change

"when placed in an entry block, only 2 incoming blocks are available");

"loop region has a single predecessor (preheader), its entry block has 2 incoming blocks");

Perhaps better have the verifier assert loop regions have a single predecessor, than here.

Updated the comment, will adjust the verifier separately, thanks!

ayalz · 2025-05-02T07:37:29Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

+    // region itself whose exiting block feeds the phi across the backedge.
+    Pred = Idx == 0 ? Region->getSinglePredecessor() : Region;
+  }
+


nit: redundant?

Suggested change

Dropped, thanks

ayalz · 2025-05-02T07:42:19Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  VPValue *getIncomingValue(unsigned Idx) const {
+    return getAsRecipe()->getOperand(Idx);
+  }


VPWidenPHIRecipe, VPHeaderPHIRecipe (base class of VPEVLBasedPhi), and VPPredInstPhi all inherit from VPSingleDefRecipe. So could inherit from VPSingleDefPhiRecipe instead, which could take care of implementing these pure virtual methods for them.

ayalz · 2025-05-03T06:52:47Z

llvm/lib/Transforms/Vectorize/VPlan.h

+template <typename RecipeTy> class VPPhiAccessors {
+  /// Return a VPRecipeBase* to the current object.
+  const VPRecipeBase *getAsRecipe() const {
+    return static_cast<const RecipeTy *>(this);


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

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?

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 *>

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

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).

ayalz · 2025-05-03T10:48:49Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  }
+
+  /// Returns the incoming block with index \p Idx.
+  const VPBasicBlock *getIncomingBlock(unsigned Idx) const;


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

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

Ok, probably better done separately, if at all.

ayalz · 2025-05-03T10:49:46Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  /// Returns the incoming block with index \p Idx.
+  const VPBasicBlock *getIncomingBlock(unsigned Idx) const;
+
+  unsigned getNumIncomingValues() const {


Suggested change

unsigned getNumIncomingValues() const {

/// Returns the number of incoming values, also number of incoming blocks.

unsigned getNumIncoming() const {

its both Values and Blocks?

Done thanks

llvm-ci · 2025-05-04T13:00:34Z

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-gcc-ubuntu running on sie-linux-worker3 while building llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/17209

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'AddressSanitizer-x86_64-linux-dynamic :: TestCases/asan_lsan_deadlock.cpp' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./bin/clang  --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only  -m64  -shared-libasan -O0 /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/compiler-rt/test/asan/TestCases/asan_lsan_deadlock.cpp -o /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/runtimes/runtimes-bins/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Output/asan_lsan_deadlock.cpp.tmp # RUN: at line 4
+ /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -shared-libasan -O0 /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/compiler-rt/test/asan/TestCases/asan_lsan_deadlock.cpp -o /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/runtimes/runtimes-bins/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Output/asan_lsan_deadlock.cpp.tmp
env ASAN_OPTIONS=detect_leaks=1 not  /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/runtimes/runtimes-bins/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Output/asan_lsan_deadlock.cpp.tmp 2>&1 | FileCheck /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/compiler-rt/test/asan/TestCases/asan_lsan_deadlock.cpp # RUN: at line 5
+ env ASAN_OPTIONS=detect_leaks=1 not /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/build/runtimes/runtimes-bins/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Output/asan_lsan_deadlock.cpp.tmp
+ FileCheck /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/compiler-rt/test/asan/TestCases/asan_lsan_deadlock.cpp
�[1m/home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/compiler-rt/test/asan/TestCases/asan_lsan_deadlock.cpp:58:12: �[0m�[0;1;31merror: �[0m�[1mCHECK: expected string not found in input
�[0m // CHECK: SUMMARY: AddressSanitizer: stack-buffer-overflow
�[0;1;32m           ^
�[0m�[1m<stdin>:1:1: �[0m�[0;1;30mnote: �[0m�[1mscanning from here
�[0m=================================================================
�[0;1;32m^
�[0m�[1m<stdin>:2:10: �[0m�[0;1;30mnote: �[0m�[1mpossible intended match here
�[0m==2699692==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7b57260de034 at pc 0x5563c852d220 bp 0x7b57242fdce0 sp 0x7b57242fdcd8
�[0;1;32m         ^
�[0m
Input file: <stdin>
Check file: /home/buildbot/buildbot-root/llvm-clang-x86_64-gcc-ubuntu/llvm-project/compiler-rt/test/asan/TestCases/asan_lsan_deadlock.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<
�[1m�[0m�[0;1;30m            1: �[0m�[1m�[0;1;46m================================================================= �[0m
�[0;1;31mcheck:58'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
�[0m�[0;1;30m            2: �[0m�[1m�[0;1;46m==2699692==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7b57260de034 at pc 0x5563c852d220 bp 0x7b57242fdce0 sp 0x7b57242fdcd8 �[0m
�[0;1;31mcheck:58'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
�[0m�[0;1;35mcheck:58'1              ?                                                                                                                                    possible intended match
�[0m�[0;1;30m            3: �[0m�[1m�[0;1;46mWRITE of size 4 at 0x7b57260de034 thread T2 �[0m
�[0;1;31mcheck:58'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
�[0m>>>>>>

--

********************

…llvm#129388) Add a VPPhiAccessors class to provide interfaces to access incoming values and blocks. The first user is VPWidenPhiRecipe, with the other phi-like recipes following soon. This will also be used to verify def-use chains where users are phi-like recipes, simplifying llvm#124838. PR: llvm#129388

…cipes (NFC) (#129388) Add a VPPhiAccessors class to provide interfaces to access incoming values and blocks. The first user is VPWidenPhiRecipe, with the other phi-like recipes following soon. This will also be used to verify def-use chains where users are phi-like recipes, simplifying llvm/llvm-project#124838. PR: llvm/llvm-project#129388

…llvm#129388) Add a VPPhiAccessors class to provide interfaces to access incoming values and blocks. The first user is VPWidenPhiRecipe, with the other phi-like recipes following soon. This will also be used to verify def-use chains where users are phi-like recipes, simplifying llvm#124838. PR: llvm#129388

fhahn requested review from lukel97, ayalz and aniragil March 1, 2025 15:57

llvmbot added vectorizers llvm:transforms labels Mar 1, 2025

fhahn mentioned this pull request Mar 1, 2025

[VPlan] Use VPPhiAccessors for VPIRPhi. #129389

Closed

iamlouk mentioned this pull request Mar 2, 2025

[LV] Optionally preserve uniform branches when vectorizing #128187

Open

lukel97 reviewed Mar 3, 2025

View reviewed changes

lukel97 approved these changes Mar 3, 2025

View reviewed changes

llvm/lib/Transforms/Vectorize/VPlan.h Outdated Show resolved Hide resolved

llvm/lib/Transforms/Vectorize/VPlan.h Outdated Show resolved Hide resolved

david-arm reviewed Mar 3, 2025

View reviewed changes

fhahn mentioned this pull request Mar 4, 2025

[VPlan] Use VPInstruction for VPScalarPHIRecipe. (NFCI) #129767

Merged

fhahn added 2 commits March 13, 2025 15:07

Merge remote-tracking branch 'origin/main' into vplan-phi-accessors

9113022

!fixup address latest comments, thanks

c61233f

Merge remote-tracking branch 'origin/main' into vplan-phi-accessors

a73a137

fhahn commented Apr 23, 2025

View reviewed changes

lukel97 reviewed Apr 24, 2025

View reviewed changes

llvm/lib/Transforms/Vectorize/VPlan.h Outdated Show resolved Hide resolved

llvm/lib/Transforms/Vectorize/VPlan.h Show resolved Hide resolved

fhahn added 2 commits April 26, 2025 21:32

Merge remote-tracking branch 'origin/main' into vplan-phi-accessors

90b2d4d

!fixup address latest comments, thanks

f768b97

ayalz reviewed Apr 28, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into vplan-phi-accessors

d6d6bb5

ayalz reviewed May 1, 2025

View reviewed changes

fhahn added 2 commits May 1, 2025 21:39

Merge remote-tracking branch 'origin/main' into vplan-phi-accessors

8dfd569

!fixup address latest comments, thanks

595b057

ayalz approved these changes May 2, 2025

View reviewed changes

ayalz reviewed May 3, 2025

View reviewed changes

fhahn added 2 commits May 3, 2025 10:40

Merge remote-tracking branch 'origin/main' into vplan-phi-accessors

a2b4ebb

!fixup address comments, make getAsRecipe pure virtual.

46780ab

ayalz reviewed May 3, 2025

View reviewed changes

!fixup address latest comments, thanks!

54981bf

fhahn merged commit 6e20519 into llvm:main May 4, 2025
9 of 11 checks passed

fhahn deleted the vplan-phi-accessors branch May 4, 2025 12:47

	"when placed in an entry block, only 2 incoming blocks are available");
	"loop region has a single predecessor (preheader), its entry block has 2 incoming blocks");

	unsigned getNumIncomingValues() const {
	/// Returns the number of incoming values, also number of incoming blocks.
	unsigned getNumIncoming() const {

[VPlan] Add VPPhiAccessors to provide interface for phi recipes (NFC) #129388

[VPlan] Add VPPhiAccessors to provide interface for phi recipes (NFC) #129388

Uh oh!

Conversation

fhahn commented Mar 1, 2025

Uh oh!

llvmbot commented Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukel97 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukel97 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

llvmbot commented Mar 1, 2025 •

edited

Loading

lukel97 left a comment •

edited

Loading