[llvm] [Utils] Consolidate `LockstepReverseIterator` into own header (NFC) (PR #116657)
Antonio Frighetto via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 13 08:07:36 PST 2024
https://github.com/antoniofrighetto updated https://github.com/llvm/llvm-project/pull/116657
>From 585631e554f60b15fd83e240ea8dc4a157a057d0 Mon Sep 17 00:00:00 2001
From: Antonio Frighetto <me at antoniofrighetto.com>
Date: Mon, 18 Nov 2024 17:54:36 +0100
Subject: [PATCH 1/2] [Utils] Consolidate `LockstepReverseIterator` into own
header (NFC)
Common code has been unified and generalized.
---
.../Utils/LockstepReverseIterator.h | 160 ++++++++++++++++++
llvm/lib/Transforms/Scalar/GVNSink.cpp | 94 +---------
llvm/lib/Transforms/Utils/SimplifyCFG.cpp | 80 +--------
3 files changed, 171 insertions(+), 163 deletions(-)
create mode 100644 llvm/include/llvm/Transforms/Utils/LockstepReverseIterator.h
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);
>From f61b17158ccc802f1038948b81b6ae95d93e67fb Mon Sep 17 00:00:00 2001
From: Antonio Frighetto <me at antoniofrighetto.com>
Date: Fri, 13 Dec 2024 17:07:07 +0100
Subject: [PATCH 2/2] !fixup address reviews
---
.../Utils/LockstepReverseIterator.h | 41 ++++++++-----------
1 file changed, 18 insertions(+), 23 deletions(-)
diff --git a/llvm/include/llvm/Transforms/Utils/LockstepReverseIterator.h b/llvm/include/llvm/Transforms/Utils/LockstepReverseIterator.h
index e5fde398f0e4c6..41223088c392e9 100644
--- a/llvm/include/llvm/Transforms/Utils/LockstepReverseIterator.h
+++ b/llvm/include/llvm/Transforms/Utils/LockstepReverseIterator.h
@@ -17,9 +17,7 @@
namespace llvm {
-struct NoActiveBlocksOption {
- template <typename... Args> NoActiveBlocksOption(Args...) {}
-};
+struct NoActiveBlocksOption {};
struct ActiveBlocksOption {
protected:
@@ -46,15 +44,12 @@ class LockstepReverseIterator
: public std::conditional_t<EarlyFailure, NoActiveBlocksOption,
ActiveBlocksOption> {
private:
- using BasicBlockT = BasicBlock;
- using InstructionT = Instruction;
-
- ArrayRef<BasicBlockT *> Blocks;
- SmallVector<InstructionT *, 4> Insts;
+ ArrayRef<BasicBlock *> Blocks;
+ SmallVector<Instruction *, 4> Insts;
bool Fail;
public:
- LockstepReverseIterator(ArrayRef<BasicBlockT *> Blocks) : Blocks(Blocks) {
+ LockstepReverseIterator(ArrayRef<BasicBlock *> Blocks) : Blocks(Blocks) {
reset();
}
@@ -62,12 +57,12 @@ class LockstepReverseIterator
Fail = false;
if constexpr (!EarlyFailure) {
this->ActiveBlocks.clear();
- for (BasicBlockT *BB : Blocks)
+ for (BasicBlock *BB : Blocks)
this->ActiveBlocks.insert(BB);
}
Insts.clear();
- for (BasicBlockT *BB : Blocks) {
- InstructionT *Prev = BB->getTerminator()->getPrevNonDebugInstruction();
+ for (BasicBlock *BB : Blocks) {
+ Instruction *Prev = BB->getTerminator()->getPrevNonDebugInstruction();
if (!Prev) {
// Block wasn't big enough - only contained a terminator.
if constexpr (EarlyFailure) {
@@ -85,7 +80,7 @@ class LockstepReverseIterator
}
bool isValid() const { return !Fail; }
- ArrayRef<InstructionT *> operator*() const { return Insts; }
+ 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
@@ -93,13 +88,13 @@ class LockstepReverseIterator
// 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() {
+ SmallSetVector<BasicBlock *, 4> &getActiveBlocks() {
+ static_assert(!EarlyFailure, "Unknown method");
return this->ActiveBlocks;
}
- template <bool C = EarlyFailure, std::enable_if_t<!C, int> = 0>
- void restrictToBlocks(SmallSetVector<BasicBlockT *, 4> &Blocks) {
+ void restrictToBlocks(SmallSetVector<BasicBlock *, 4> &Blocks) {
+ static_assert(!EarlyFailure, "Unknown method");
for (auto It = Insts.begin(); It != Insts.end();) {
if (!Blocks.contains((*It)->getParent())) {
this->ActiveBlocks.remove((*It)->getParent());
@@ -113,9 +108,9 @@ class LockstepReverseIterator
void operator--() {
if (Fail)
return;
- SmallVector<InstructionT *, 4> NewInsts;
- for (InstructionT *Inst : Insts) {
- InstructionT *Prev = Inst->getPrevNonDebugInstruction();
+ SmallVector<Instruction *, 4> NewInsts;
+ for (Instruction *Inst : Insts) {
+ Instruction *Prev = Inst->getPrevNonDebugInstruction();
if (!Prev) {
if constexpr (!EarlyFailure) {
this->ActiveBlocks.remove(Inst->getParent());
@@ -137,9 +132,9 @@ class LockstepReverseIterator
void operator++() {
if (Fail)
return;
- SmallVector<InstructionT *, 4> NewInsts;
- for (InstructionT *Inst : Insts) {
- InstructionT *Next = Inst->getNextNonDebugInstruction();
+ SmallVector<Instruction *, 4> NewInsts;
+ for (Instruction *Inst : Insts) {
+ Instruction *Next = Inst->getNextNonDebugInstruction();
// Already at end of block.
if (!Next) {
Fail = true;
More information about the llvm-commits
mailing list