[NFC][LoopVectorize] Make replaceVPBBWithIRVPBB more efficient #111514

david-arm · 2024-10-08T10:13:12Z

In replaceVPBBWithIRVPBB we spend time erasing and appending predecessors and successors from a list, when all we really have to do is replace the old with the new. Not only is this more efficient, but it also preserves the ordering of successors and predecessors. This is something which may become important for vectorising early exit loops (see PR #88385), since a VPIRInstruction is the wrapper for a live-out phi with extra operands that map to the incoming block according to the block's predecessor.

In replaceVPBBWithIRVPBB we spend time erasing and appending predecessors and successors from a list, when all we really have to do is replace the old with the new. Not only is this more efficient, but it also preserves the ordering of successors and predecessors. This is something which may become important for vectorising early exit loops (see PR llvm#88385), since a VPIRInstruction is the wrapper for a live-out phi with extra operands that map to the incoming block according to the block's predecessor.

llvmbot · 2024-10-08T10:13:49Z

@llvm/pr-subscribers-llvm-transforms

Author: David Sherwood (david-arm)

Changes

In replaceVPBBWithIRVPBB we spend time erasing and appending predecessors and successors from a list, when all we really have to do is replace the old with the new. Not only is this more efficient, but it also preserves the ordering of successors and predecessors. This is something which may become important for vectorising early exit loops (see PR #88385), since a VPIRInstruction is the wrapper for a live-out phi with extra operands that map to the incoming block according to the block's predecessor.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/VPlan.cpp (+8-6)
(modified) llvm/lib/Transforms/Vectorize/VPlan.h (+20)

diff --git a/llvm/lib/Transforms/Vectorize/VPlan.cpp b/llvm/lib/Transforms/Vectorize/VPlan.cpp
index 5e3a6388094940..530c55bfe8e74f 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlan.cpp
@@ -1005,12 +1005,14 @@ static void replaceVPBBWithIRVPBB(VPBasicBlock *VPBB, BasicBlock *IRBB) {
     R.moveBefore(*IRVPBB, IRVPBB->end());
   }
   VPBlockBase *PredVPBB = VPBB->getSinglePredecessor();
-  VPBlockUtils::disconnectBlocks(PredVPBB, VPBB);
-  VPBlockUtils::connectBlocks(PredVPBB, IRVPBB);
-  for (auto *Succ : to_vector(VPBB->getSuccessors())) {
-    VPBlockUtils::connectBlocks(IRVPBB, Succ);
-    VPBlockUtils::disconnectBlocks(VPBB, Succ);
-  }
+  PredVPBB->replaceSuccessor(VPBB, IRVPBB);
+  IRVPBB->setPredecessors({PredVPBB});
+  for (auto *Succ : to_vector(VPBB->getSuccessors()))
+    Succ->replacePredecessor(VPBB, IRVPBB);
+  IRVPBB->setSuccessors(VPBB->getSuccessors());
+
+  VPBB->clearSuccessors();
+  VPBB->clearPredecessors();
   delete VPBB;
 }
 
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 8c5246d613c13d..88cb8e47cc9278 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -556,6 +556,26 @@ class VPBlockBase {
     return getEnclosingBlockWithPredecessors()->getSinglePredecessor();
   }
 
+  /// This function replaces one predecessor with another, useful when
+  /// trying to replace an old block in the CFG with a new one.
+  void replacePredecessor(VPBlockBase *Old, VPBlockBase *New) {
+    auto I = find(Predecessors, Old);
+    assert(I != Predecessors.end());
+    assert(Old->getParent() == New->getParent() &&
+           "replaced predecessor must have the same parent");
+    *I = New;
+  }
+
+  /// This function replaces one successor with another, useful when
+  /// trying to replace an old block in the CFG with a new one.
+  void replaceSuccessor(VPBlockBase *Old, VPBlockBase *New) {
+    auto I = find(Successors, Old);
+    assert(I != Successors.end());
+    assert(Old->getParent() == New->getParent() &&
+           "replaced successor must have the same parent");
+    *I = New;
+  }
+
   /// Set a given VPBlockBase \p Successor as the single successor of this
   /// VPBlockBase. This VPBlockBase is not added as predecessor of \p Successor.
   /// This VPBlockBase must have no successors.

paulwalker-arm

I don't know enough to know if maintaining the block ordering is what we need/want from a design point of view (it seems kind of fragile to me) but I've added some comments assuming the efficiency gains are worth it.

llvm/lib/Transforms/Vectorize/VPlan.cpp

paulwalker-arm · 2024-10-10T11:56:34Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

-    VPBlockUtils::disconnectBlocks(VPBB, Succ);
-  }
+  PredVPBB->replaceSuccessor(VPBB, IRVPBB);
+  IRVPBB->setPredecessors({PredVPBB});


Can you be sure that for all future uses of this function IRVPBB's predecessors and successors can be replaced? Would it be safer to append them as was done previously?

From a structural point of view VPBlockUtils looks to be the place for managing VPBB linkage (I see a range of insert methods along with the (dis)connect method you're replacing) so in keeping with this perhaps add VPBlockUtils::reassociateBlock(Keep, Old, New) that calls onto replaceSuccessor and replacePredecessor.

Good suggestion - I've added a new VPBlockUtils::reassociateBlock function and do the replacements in there!

fhahn

Thanks for putting up the patch!

The order of successors and predecessors is meaningful in VPlan, e.g. to determine the order of operands in a branch instruction for VPBlocks with multiple successors. So it should definitely be preserved here, it wasn't an issue so far as the middle block always had a single predecessor

llvm/lib/Transforms/Vectorize/VPlan.cpp

* Add new VPBlockUtils::reassociateBlocks that replaces all associations to Old with New. * Removed code to clear successors/predecessors in the deleted block.

fhahn · 2024-10-13T19:18:48Z

llvm/lib/Transforms/Vectorize/VPlan.h

+    assert(I != Predecessors.end());
+    assert(Old->getParent() == New->getParent() &&
+           "replaced predecessor must have the same parent");
+    *I = New;


I think this may leaves Old in an invalid state, as we remove it from the list of predecessors here, but don't remove this from the Successors of Old. Can we also remove it from there? Otherwise the callers of the API must either remove the node itself or completely delete the block

Hmm, but if we go down that route then surely we should also care about setting the successor of New to this as well so that we leave it in a valid state? I guess if this function was private then it wouldn't be part of the interface so it wouldn't matter as much. If we do the extra work here as you suggest, then this patch becomes less about efficiency and more about retaining ordering, which is fine but then I probably ought to change the title!

For example, removePredecessor does not leave the Predecessor argument in a valid state either because this will still be a successor of Predecessor . How about I make the functions private and then we can behave like removePredecessor, unless you see value in these new interfaces being public?

Right, let me take a look tomorrow

Sure, no problem. I just added a new commit as I realised the the public reassociateBlocks interface was leaving Old and New in an invalid state. Hopefully, this now addresses your concern!

Thanks this was what I was about to suggest, as this ensures that reassociateBlocks leaves the CFG in a valid state.

* Made functions private in same way as removeSuccessor and removePredecessor so that we no longer have to worry about maintaining a valid state of `Old`.

* Since reassociateBlocks is a public interface we should leave the Old and New blocks in a usable state.

fhahn

LGTM, thanks!

…111514) In replaceVPBBWithIRVPBB we spend time erasing and appending predecessors and successors from a list, when all we really have to do is replace the old with the new. Not only is this more efficient, but it also preserves the ordering of successors and predecessors. This is something which may become important for vectorising early exit loops (see PR llvm#88385), since a VPIRInstruction is the wrapper for a live-out phi with extra operands that map to the incoming block according to the block's predecessor.

ayalz

Various post-commit comments.

ayalz · 2024-10-25T15:31:32Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  /// Reassociate all the blocks connected to \p Old so that they now point to
+  /// \p New.
+  static void reassociateBlocks(VPBlockBase *Old, VPBlockBase *New) {
+    for (auto *Pred : to_vector(Old->getPredecessors()))


Is this to_vector() needed, given that Old's predecessors remain intact?

ayalz · 2024-10-25T15:32:00Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  static void reassociateBlocks(VPBlockBase *Old, VPBlockBase *New) {
+    for (auto *Pred : to_vector(Old->getPredecessors()))
+      Pred->replaceSuccessor(Old, New);
+    for (auto *Succ : to_vector(Old->getSuccessors()))


Is this to_vector() needed, given that Old's successors remain intact?

ayalz · 2024-10-25T15:33:06Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -3862,6 +3882,19 @@ class VPBlockUtils {
    To->removePredecessor(From);
  }

+  /// Reassociate all the blocks connected to \p Old so that they now point to
+  /// \p New.


Suggested change

/// \p New.

/// \p New instead.

ayalz · 2024-10-25T15:33:28Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  /// trying to replace an old block in the CFG with a new one.
+  void replaceSuccessor(VPBlockBase *Old, VPBlockBase *New) {
+    auto I = find(Successors, Old);
+    assert(I != Successors.end());


Error message missing.

ayalz · 2024-10-25T15:36:11Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

@@ -1004,13 +1004,9 @@ static void replaceVPBBWithIRVPBB(VPBasicBlock *VPBB, BasicBlock *IRBB) {
    assert(!R.isPhi() && "Tried to move phi recipe to end of block");


While we're here, worth adding in documentation above that VPBB, having a single predecessor, is expected to be free of phi recipes. A more informative error message may be "VPBB to be replaced by IRBB must be free of phi recipes".

Note that, in general, replacing a VPBB with an IRBB is a temporary solution, until the full skeleton is built in VPlan. Blocks would clearly be identified as IRBB's or not, from the outset.

ayalz · 2024-10-25T15:41:24Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

@@ -1004,13 +1004,9 @@ static void replaceVPBBWithIRVPBB(VPBasicBlock *VPBB, BasicBlock *IRBB) {
    assert(!R.isPhi() && "Tried to move phi recipe to end of block");
    R.moveBefore(*IRVPBB, IRVPBB->end());
  }
-  VPBlockBase *PredVPBB = VPBB->getSinglePredecessor();


This assert of having a single predecessor is now lost, yet still documented. The function traverses multiple predecessors, but probably works and tested for single predecessor only, as VPBB is still expected to be free of phi recipes? Better be clear, assert and test what is supported.

ayalz · 2024-10-25T15:44:13Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  /// trying to replace an old block in the CFG with a new one.
+  void replacePredecessor(VPBlockBase *Old, VPBlockBase *New) {
+    auto I = find(Predecessors, Old);
+    assert(I != Predecessors.end());


Error message missing.

david-arm requested review from fhahn, paulwalker-arm and huntergr-arm October 8, 2024 10:13

llvmbot added vectorizers llvm:transforms labels Oct 8, 2024

david-arm mentioned this pull request Oct 9, 2024

[VPlan] Introduce scalar loop header in plan, remove VPLiveOut. #109975

Merged

paulwalker-arm reviewed Oct 10, 2024

View reviewed changes

fhahn reviewed Oct 10, 2024

View reviewed changes

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

Address review comments

6c45224

* Add new VPBlockUtils::reassociateBlocks that replaces all associations to Old with New. * Removed code to clear successors/predecessors in the deleted block.

fhahn reviewed Oct 13, 2024

View reviewed changes

david-arm added 2 commits October 14, 2024 07:51

Make new replaceXYZ functions private

3da4e4e

* Made functions private in same way as removeSuccessor and removePredecessor so that we no longer have to worry about maintaining a valid state of `Old`.

Ensure reassociateBlocks leaves Old and New in a decent state

d1b45da

* Since reassociateBlocks is a public interface we should leave the Old and New blocks in a usable state.

fhahn approved these changes Oct 15, 2024

View reviewed changes

david-arm merged commit 175461a into llvm:main Oct 15, 2024
6 of 8 checks passed

ayalz reviewed Oct 25, 2024

View reviewed changes

david-arm deleted the replace_vpbb branch January 28, 2025 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NFC][LoopVectorize] Make replaceVPBBWithIRVPBB more efficient #111514

[NFC][LoopVectorize] Make replaceVPBBWithIRVPBB more efficient #111514

david-arm commented Oct 8, 2024

llvmbot commented Oct 8, 2024

paulwalker-arm left a comment

paulwalker-arm Oct 10, 2024

david-arm Oct 11, 2024

fhahn left a comment

fhahn Oct 13, 2024

david-arm Oct 14, 2024

david-arm Oct 14, 2024 •

edited

Loading

fhahn Oct 14, 2024

david-arm Oct 15, 2024

fhahn Oct 15, 2024

fhahn left a comment

ayalz left a comment

ayalz Oct 25, 2024

ayalz Oct 25, 2024

ayalz Oct 25, 2024

ayalz Oct 25, 2024

ayalz Oct 25, 2024

ayalz Oct 25, 2024

ayalz Oct 25, 2024

		@@ -1004,13 +1004,9 @@ static void replaceVPBBWithIRVPBB(VPBasicBlock VPBB, BasicBlock IRBB) {
		assert(!R.isPhi() && "Tried to move phi recipe to end of block");

[NFC][LoopVectorize] Make replaceVPBBWithIRVPBB more efficient #111514

[NFC][LoopVectorize] Make replaceVPBBWithIRVPBB more efficient #111514

Conversation

david-arm commented Oct 8, 2024

llvmbot commented Oct 8, 2024

paulwalker-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-arm Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhahn left a comment

Choose a reason for hiding this comment

ayalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-arm Oct 14, 2024 •

edited

Loading