[Utils] Consolidate LockstepReverseIterator into own header (NFC)
#116657
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Common code has been unified and generalized.
|
@llvm/pr-subscribers-llvm-transforms Author: Antonio Frighetto (antoniofrighetto) ChangesCommon code has been unified and generalized. Not sure if it may be worth to generalize this further, since it looks closely tied Blocks (might make sense to rename it in Full diff: https://github.com/llvm/llvm-project/pull/116657.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Utils/LockstepReverseIterator.h b/llvm/include/llvm/Transforms/Utils/LockstepReverseIterator.h
new file mode 100644
index 00000000000000..e5fde398f0e4c6
--- /dev/null
+++ b/llvm/include/llvm/Transforms/Utils/LockstepReverseIterator.h
@@ -0,0 +1,160 @@
+//===- LockstepReverseIterator.h ------------------------------*- C++ -*---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TRANSFORMS_UTILS_LOCKSTEPREVERSEITERATOR_H
+#define LLVM_TRANSFORMS_UTILS_LOCKSTEPREVERSEITERATOR_H
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/SetVector.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/Instruction.h"
+
+namespace llvm {
+
+struct NoActiveBlocksOption {
+ template <typename... Args> NoActiveBlocksOption(Args...) {}
+};
+
+struct ActiveBlocksOption {
+protected:
+ SmallSetVector<BasicBlock *, 4> ActiveBlocks;
+
+public:
+ ActiveBlocksOption() = default;
+};
+
+/// Iterates through instructions in a set of blocks in reverse order from the
+/// first non-terminator. For example (assume all blocks have size n):
+/// LockstepReverseIterator I([B1, B2, B3]);
+/// *I-- = [B1[n], B2[n], B3[n]];
+/// *I-- = [B1[n-1], B2[n-1], B3[n-1]];
+/// *I-- = [B1[n-2], B2[n-2], B3[n-2]];
+/// ...
+///
+/// The iterator continues processing until all blocks have been exhausted if \p
+/// EarlyFailure is explicitly set to \c false. Use \c getActiveBlocks() to
+/// determine which blocks are still going and the order they appear in the list
+/// returned by operator*.
+template <bool EarlyFailure = true>
+class LockstepReverseIterator
+ : public std::conditional_t<EarlyFailure, NoActiveBlocksOption,
+ ActiveBlocksOption> {
+private:
+ using BasicBlockT = BasicBlock;
+ using InstructionT = Instruction;
+
+ ArrayRef<BasicBlockT *> Blocks;
+ SmallVector<InstructionT *, 4> Insts;
+ bool Fail;
+
+public:
+ LockstepReverseIterator(ArrayRef<BasicBlockT *> Blocks) : Blocks(Blocks) {
+ reset();
+ }
+
+ void reset() {
+ Fail = false;
+ if constexpr (!EarlyFailure) {
+ this->ActiveBlocks.clear();
+ for (BasicBlockT *BB : Blocks)
+ this->ActiveBlocks.insert(BB);
+ }
+ Insts.clear();
+ for (BasicBlockT *BB : Blocks) {
+ InstructionT *Prev = BB->getTerminator()->getPrevNonDebugInstruction();
+ if (!Prev) {
+ // Block wasn't big enough - only contained a terminator.
+ if constexpr (EarlyFailure) {
+ Fail = true;
+ return;
+ } else {
+ this->ActiveBlocks.remove(BB);
+ continue;
+ }
+ }
+ Insts.push_back(Prev);
+ }
+ if (Insts.empty())
+ Fail = true;
+ }
+
+ bool isValid() const { return !Fail; }
+ ArrayRef<InstructionT *> operator*() const { return Insts; }
+
+ // Note: This needs to return a SmallSetVector as the elements of
+ // ActiveBlocks will be later copied to Blocks using std::copy. The
+ // resultant order of elements in Blocks needs to be deterministic.
+ // Using SmallPtrSet instead causes non-deterministic order while
+ // copying. And we cannot simply sort Blocks as they need to match the
+ // corresponding Values.
+ template <bool C = EarlyFailure, std::enable_if_t<!C, int> = 0>
+ SmallSetVector<BasicBlockT *, 4> &getActiveBlocks() {
+ return this->ActiveBlocks;
+ }
+
+ template <bool C = EarlyFailure, std::enable_if_t<!C, int> = 0>
+ void restrictToBlocks(SmallSetVector<BasicBlockT *, 4> &Blocks) {
+ for (auto It = Insts.begin(); It != Insts.end();) {
+ if (!Blocks.contains((*It)->getParent())) {
+ this->ActiveBlocks.remove((*It)->getParent());
+ It = Insts.erase(It);
+ } else {
+ ++It;
+ }
+ }
+ }
+
+ void operator--() {
+ if (Fail)
+ return;
+ SmallVector<InstructionT *, 4> NewInsts;
+ for (InstructionT *Inst : Insts) {
+ InstructionT *Prev = Inst->getPrevNonDebugInstruction();
+ if (!Prev) {
+ if constexpr (!EarlyFailure) {
+ this->ActiveBlocks.remove(Inst->getParent());
+ } else {
+ Fail = true;
+ return;
+ }
+ } else {
+ NewInsts.push_back(Prev);
+ }
+ }
+ if (NewInsts.empty()) {
+ Fail = true;
+ return;
+ }
+ Insts = NewInsts;
+ }
+
+ void operator++() {
+ if (Fail)
+ return;
+ SmallVector<InstructionT *, 4> NewInsts;
+ for (InstructionT *Inst : Insts) {
+ InstructionT *Next = Inst->getNextNonDebugInstruction();
+ // Already at end of block.
+ if (!Next) {
+ Fail = true;
+ return;
+ }
+ NewInsts.push_back(Next);
+ }
+ if (NewInsts.empty()) {
+ Fail = true;
+ return;
+ }
+ Insts = NewInsts;
+ }
+};
+
+} // end namespace llvm
+
+#endif // LLVM_TRANSFORMS_UTILS_LOCKSTEPREVERSEITERATOR_H
diff --git a/llvm/lib/Transforms/Scalar/GVNSink.cpp b/llvm/lib/Transforms/Scalar/GVNSink.cpp
index 730f5cd0f8d0d7..8fc09db668fe43 100644
--- a/llvm/lib/Transforms/Scalar/GVNSink.cpp
+++ b/llvm/lib/Transforms/Scalar/GVNSink.cpp
@@ -65,6 +65,7 @@
#include "llvm/Transforms/Scalar/GVNExpression.h"
#include "llvm/Transforms/Utils/BasicBlockUtils.h"
#include "llvm/Transforms/Utils/Local.h"
+#include "llvm/Transforms/Utils/LockstepReverseIterator.h"
#include <cassert>
#include <cstddef>
#include <cstdint>
@@ -96,87 +97,6 @@ static bool isMemoryInst(const Instruction *I) {
(isa<CallInst>(I) && !cast<CallInst>(I)->doesNotAccessMemory());
}
-/// Iterates through instructions in a set of blocks in reverse order from the
-/// first non-terminator. For example (assume all blocks have size n):
-/// LockstepReverseIterator I([B1, B2, B3]);
-/// *I-- = [B1[n], B2[n], B3[n]];
-/// *I-- = [B1[n-1], B2[n-1], B3[n-1]];
-/// *I-- = [B1[n-2], B2[n-2], B3[n-2]];
-/// ...
-///
-/// It continues until all blocks have been exhausted. Use \c getActiveBlocks()
-/// to
-/// determine which blocks are still going and the order they appear in the
-/// list returned by operator*.
-class LockstepReverseIterator {
- ArrayRef<BasicBlock *> Blocks;
- SmallSetVector<BasicBlock *, 4> ActiveBlocks;
- SmallVector<Instruction *, 4> Insts;
- bool Fail;
-
-public:
- LockstepReverseIterator(ArrayRef<BasicBlock *> Blocks) : Blocks(Blocks) {
- reset();
- }
-
- void reset() {
- Fail = false;
- ActiveBlocks.clear();
- for (BasicBlock *BB : Blocks)
- ActiveBlocks.insert(BB);
- Insts.clear();
- for (BasicBlock *BB : Blocks) {
- if (BB->size() <= 1) {
- // Block wasn't big enough - only contained a terminator.
- ActiveBlocks.remove(BB);
- continue;
- }
- Insts.push_back(BB->getTerminator()->getPrevNonDebugInstruction());
- }
- if (Insts.empty())
- Fail = true;
- }
-
- bool isValid() const { return !Fail; }
- ArrayRef<Instruction *> operator*() const { return Insts; }
-
- // Note: This needs to return a SmallSetVector as the elements of
- // ActiveBlocks will be later copied to Blocks using std::copy. The
- // resultant order of elements in Blocks needs to be deterministic.
- // Using SmallPtrSet instead causes non-deterministic order while
- // copying. And we cannot simply sort Blocks as they need to match the
- // corresponding Values.
- SmallSetVector<BasicBlock *, 4> &getActiveBlocks() { return ActiveBlocks; }
-
- void restrictToBlocks(SmallSetVector<BasicBlock *, 4> &Blocks) {
- for (auto II = Insts.begin(); II != Insts.end();) {
- if (!Blocks.contains((*II)->getParent())) {
- ActiveBlocks.remove((*II)->getParent());
- II = Insts.erase(II);
- } else {
- ++II;
- }
- }
- }
-
- void operator--() {
- if (Fail)
- return;
- SmallVector<Instruction *, 4> NewInsts;
- for (auto *Inst : Insts) {
- if (Inst == &Inst->getParent()->front())
- ActiveBlocks.remove(Inst->getParent());
- else
- NewInsts.push_back(Inst->getPrevNonDebugInstruction());
- }
- if (NewInsts.empty()) {
- Fail = true;
- return;
- }
- Insts = NewInsts;
- }
-};
-
//===----------------------------------------------------------------------===//
/// Candidate solution for sinking. There may be different ways to
@@ -634,9 +554,11 @@ class GVNSink {
/// The main heuristic function. Analyze the set of instructions pointed to by
/// LRI and return a candidate solution if these instructions can be sunk, or
/// std::nullopt otherwise.
- std::optional<SinkingInstructionCandidate> analyzeInstructionForSinking(
- LockstepReverseIterator &LRI, unsigned &InstNum, unsigned &MemoryInstNum,
- ModelledPHISet &NeededPHIs, SmallPtrSetImpl<Value *> &PHIContents);
+ std::optional<SinkingInstructionCandidate>
+ analyzeInstructionForSinking(LockstepReverseIterator<false> &LRI,
+ unsigned &InstNum, unsigned &MemoryInstNum,
+ ModelledPHISet &NeededPHIs,
+ SmallPtrSetImpl<Value *> &PHIContents);
/// Create a ModelledPHI for each PHI in BB, adding to PHIs.
void analyzeInitialPHIs(BasicBlock *BB, ModelledPHISet &PHIs,
@@ -675,7 +597,7 @@ class GVNSink {
};
std::optional<SinkingInstructionCandidate>
-GVNSink::analyzeInstructionForSinking(LockstepReverseIterator &LRI,
+GVNSink::analyzeInstructionForSinking(LockstepReverseIterator<false> &LRI,
unsigned &InstNum,
unsigned &MemoryInstNum,
ModelledPHISet &NeededPHIs,
@@ -827,7 +749,7 @@ unsigned GVNSink::sinkBB(BasicBlock *BBEnd) {
return BB->getTerminator()->getNumSuccessors() != 1;
});
- LockstepReverseIterator LRI(Preds);
+ LockstepReverseIterator<false> LRI(Preds);
SmallVector<SinkingInstructionCandidate, 4> Candidates;
unsigned InstNum = 0, MemoryInstNum = 0;
ModelledPHISet NeededPHIs;
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 1991ec82d1e1e4..7440ca46c8d27d 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -74,6 +74,7 @@
#include "llvm/Support/raw_ostream.h"
#include "llvm/Transforms/Utils/BasicBlockUtils.h"
#include "llvm/Transforms/Utils/Local.h"
+#include "llvm/Transforms/Utils/LockstepReverseIterator.h"
#include "llvm/Transforms/Utils/ValueMapper.h"
#include <algorithm>
#include <cassert>
@@ -2326,81 +2327,6 @@ static void sinkLastInstruction(ArrayRef<BasicBlock*> Blocks) {
}
}
-namespace {
-
- // LockstepReverseIterator - Iterates through instructions
- // in a set of blocks in reverse order from the first non-terminator.
- // For example (assume all blocks have size n):
- // LockstepReverseIterator I([B1, B2, B3]);
- // *I-- = [B1[n], B2[n], B3[n]];
- // *I-- = [B1[n-1], B2[n-1], B3[n-1]];
- // *I-- = [B1[n-2], B2[n-2], B3[n-2]];
- // ...
- class LockstepReverseIterator {
- ArrayRef<BasicBlock*> Blocks;
- SmallVector<Instruction*,4> Insts;
- bool Fail;
-
- public:
- LockstepReverseIterator(ArrayRef<BasicBlock*> Blocks) : Blocks(Blocks) {
- reset();
- }
-
- void reset() {
- Fail = false;
- Insts.clear();
- for (auto *BB : Blocks) {
- Instruction *Inst = BB->getTerminator();
- for (Inst = Inst->getPrevNode(); Inst && isa<DbgInfoIntrinsic>(Inst);)
- Inst = Inst->getPrevNode();
- if (!Inst) {
- // Block wasn't big enough.
- Fail = true;
- return;
- }
- Insts.push_back(Inst);
- }
- }
-
- bool isValid() const {
- return !Fail;
- }
-
- void operator--() {
- if (Fail)
- return;
- for (auto *&Inst : Insts) {
- for (Inst = Inst->getPrevNode(); Inst && isa<DbgInfoIntrinsic>(Inst);)
- Inst = Inst->getPrevNode();
- // Already at beginning of block.
- if (!Inst) {
- Fail = true;
- return;
- }
- }
- }
-
- void operator++() {
- if (Fail)
- return;
- for (auto *&Inst : Insts) {
- for (Inst = Inst->getNextNode(); Inst && isa<DbgInfoIntrinsic>(Inst);)
- Inst = Inst->getNextNode();
- // Already at end of block.
- if (!Inst) {
- Fail = true;
- return;
- }
- }
- }
-
- ArrayRef<Instruction*> operator * () const {
- return Insts;
- }
- };
-
-} // end anonymous namespace
-
/// Check whether BB's predecessors end with unconditional branches. If it is
/// true, sink any common code from the predecessors to BB.
static bool sinkCommonCodeFromPredecessors(BasicBlock *BB,
@@ -2477,7 +2403,7 @@ static bool sinkCommonCodeFromPredecessors(BasicBlock *BB,
int ScanIdx = 0;
SmallPtrSet<Value*,4> InstructionsToSink;
- LockstepReverseIterator LRI(UnconditionalPreds);
+ LockstepReverseIterator<true> LRI(UnconditionalPreds);
while (LRI.isValid() &&
canSinkInstructions(*LRI, PHIOperands)) {
LLVM_DEBUG(dbgs() << "SINK: instruction can be sunk: " << *(*LRI)[0]
@@ -2507,7 +2433,7 @@ static bool sinkCommonCodeFromPredecessors(BasicBlock *BB,
// Okay, we *could* sink last ScanIdx instructions. But how many can we
// actually sink before encountering instruction that is unprofitable to
// sink?
- auto ProfitableToSinkInstruction = [&](LockstepReverseIterator &LRI) {
+ auto ProfitableToSinkInstruction = [&](LockstepReverseIterator<true> &LRI) {
unsigned NumPHIInsts = 0;
for (Use &U : (*LRI)[0]->operands()) {
auto It = PHIOperands.find(&U);
|
|
Kind ping. |
dtcxzyw
reviewed
Dec 13, 2024
| template <typename... Args> NoActiveBlocksOption(Args...) {} | ||
| }; | ||
|
|
||
| struct ActiveBlocksOption { |
There was a problem hiding this comment.
I'd rather favour struct for consistency, as we keep the default constructor private for NoActiveBlocksOption, unless it's a strong concern.
There was a problem hiding this comment.
FWIW https://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords says to use struct when all members are public, which isn't the case here
|
Gentle ping. |
dtcxzyw
approved these changes
Feb 14, 2025
| Insts = NewInsts; | ||
| } | ||
|
|
||
| void operator++() { |
There was a problem hiding this comment.
Suggested change
| void operator++() { | |
| void operator++() { | |
| static_assert(!EarlyFailure, "Unknown method"); |
This method does not maintain the set of active blocks.
dwblaikie
reviewed
Feb 14, 2025
| template <typename... Args> NoActiveBlocksOption(Args...) {} | ||
| }; | ||
|
|
||
| struct ActiveBlocksOption { |
There was a problem hiding this comment.
FWIW https://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords says to use struct when all members are public, which isn't the case here
Comment on lines
44
to
45
| : public std::conditional_t<EarlyFailure, NoActiveBlocksOption, | ||
| ActiveBlocksOption> { |
There was a problem hiding this comment.
perhaps this should use private inheritance? (then the bases could both be structs with public members, since they'd become private here anyway?)
There was a problem hiding this comment.
Switched to private and using structs is justified now, thanks!
Comment on lines
91
to
95
| SmallSetVector<BasicBlock *, 4> &getActiveBlocks() { | ||
| static_assert(!EarlyFailure, "Unknown method"); | ||
| return this->ActiveBlocks; | ||
| } | ||
|
|
There was a problem hiding this comment.
Could this be a member of the active blocks base, so it's just totally not defined on the inactive version? (then there'd be no need for the static assert, and maybe it'd be better for compiler diagnostics?)
There was a problem hiding this comment.
Makes sense, though we still need to expose getActiveBlocks in the derived as well, as the base one is now constrained by private inheritance.
| } | ||
| } | ||
|
|
||
| void operator--() { |
There was a problem hiding this comment.
Shouldn't this an op++ return iterators?
There was a problem hiding this comment.
Fixed, thanks!
dwblaikie
approved these changes
Feb 21, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/18601 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/15287 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/20624 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/185/builds/13529 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/13768 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/175/builds/13585 Here is the relevant piece of the build log for the reference |
fhahn
added a commit
that referenced
this pull request
Feb 21, 2025
… (NFC) (#116657)" This reverts commit 123dca9. This breaks building on macOS with clang and multiple build bots, including https://lab.llvm.org/buildbot/#/builders/175/builds/13585 llvm-project/llvm/lib/Transforms/Utils/SimplifyCFG.cpp: In function ‘bool sinkCommonCodeFromPredecessors(llvm::BasicBlock*, llvm::DomTreeUpdater*)’: /b/ml-opt-devrel-x86-64-b1/llvm-project/llvm/lib/Transforms/Utils/SimplifyCFG.cpp:2503:3: error: reference to ‘LockstepReverseIterator’ is ambiguous 2503 | LockstepReverseIterator<true> LRI(UnconditionalPreds); | ^~~~~~~~~~~~~~~~~~~~~~~
|
Unfortunately this breaks building on macOS with Clang and multiple build bots. I reverted the change for now in 236fa50 to get things back to building. |
Thank you, this (I think) broke all our builds. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/2/builds/17612 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/13363 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/13366 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/92/builds/14046 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/33/builds/11744 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/23564 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/19156 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/20165 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/14174 Here is the relevant piece of the build log for the reference |
Thanks, I'm taking a look! |
|
What occurred here seems quite weird: original commit 585631e as part of this PR correctly drops the class in SimplifyCFG, but the landed commit (123dca9) merged by David seems to have the LockstepReverseIterator class restored in SimplifyCFG. I was assuming something went wrong while merging this, however, unbeknownst to me, this happened in 48a6df3 too, despite being rebased to main. Should have been fixed separately in 93b263a. |
|
I think your reapplied attempt still has a build failure: I would have written this comment at the PR of the reapply commit - but it didn't have a PR -.- One benefit of a PR is to have pre-merge bots checking if the code compiles, avoiding similar mistakes. |
Fixed in 93b263a. The pre-merge checks had been already carried out within this PR, and so has been done locally running expensive checks for the reapply commit. The issue would have not been caught either with a new PR, just as it happened when David merged this the first time (despite bots being green). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Common code has been unified and generalized. Not sure if it may be worth to generalize this further, since it looks closely tied to Blocks (might make sense to rename it in
LockstepReverseInstructionIterator).