[llvm] [SandboxVec] Add barebones Region class. (PR #108899)
Jorge Gorbe Moya via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 17 14:39:25 PDT 2024
https://github.com/slackito updated https://github.com/llvm/llvm-project/pull/108899
>From a0a130aef696ba3bec579ac8966703e20d6f73d7 Mon Sep 17 00:00:00 2001
From: Jorge Gorbe Moya <jgorbe at google.com>
Date: Mon, 16 Sep 2024 15:37:32 -0700
Subject: [PATCH 1/5] [SandboxVec] Add barebones Region class.
A region identifies a set of vector instructions generated by
vectorization passes. The vectorizer can then run a series of
RegionPasses on the region, evaluate the cost, and commit/reject the
transforms on a region-by-region basis, instead of an entire basic
block.
This is heavily based ov @vporpo's prototype. In particular, the doc
comment for the Region class is all his. The rest of this commit is
mostly boilerplate around a SetVector: getters, iterators, and some
debug helpers.
---
.../Vectorize/SandboxVectorizer/Region.h | 127 ++++++++++++++++++
llvm/lib/Transforms/Vectorize/CMakeLists.txt | 1 +
.../Vectorize/SandboxVectorizer/Region.cpp | 52 +++++++
3 files changed, 180 insertions(+)
create mode 100644 llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h
create mode 100644 llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h
new file mode 100644
index 00000000000000..06c432c2c7b839
--- /dev/null
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h
@@ -0,0 +1,127 @@
+//===- Region.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_VECTORIZE_SANDBOXVECTORIZER_REGION_H
+#define LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_REGION_H
+
+#include "llvm/ADT/SetVector.h"
+#include "llvm/SandboxIR/SandboxIR.h"
+#include "llvm/Support/InstructionCost.h"
+#include "llvm/Support/raw_ostream.h"
+
+namespace llvm {
+namespace sandboxir {
+
+/// The main job of the Region is to point to new instructions generated by
+/// vectorization passes. It is the unit that RegionPasses operate on with their
+/// runOnRegion() function.
+///
+/// The region allows us to stack transformations horizontally, meaning that
+/// each transformation operates on a single region and the resulting region is
+/// the input to the next transformation, as opposed to vertically, which is the
+/// common way of applying a transformation across the whole BB. This enables us
+/// to check for profitability and decide whether we accept or rollback at a
+/// region granularity, which is much better than doing this at the BB level.
+///
+/// The region keeps track of the costs that correspond to that region. That is
+/// the original "scalar" cost and the new "vector" cost. The cost is updated
+/// automatically via SandboxIR callbacks:
+/// (i) Upon deletion: If the deleted instruction is not in the region, it is an
+/// original scalar, so its cost is added to the "scalar cost", otherwise it is
+/// a "vector" instruction created by the vectorizer so we subtract its cost
+/// from the "vector cost".
+/// (ii) Upon creation: New instructions are always considered "vector" so its
+/// cost is added to the "vector cost".
+/// This cost tracking allows us to start with transformations that may not be
+/// profitable if applied by themselves, but may end up being profitable after a
+/// series of transformations.
+///
+// Traditional approach: transformations applied vertically for the whole BB
+// BB
+// +----+
+// | |
+// | |
+// | | -> Transform1 -> ... -> TransformN -> Check Cost
+// | |
+// | |
+// +----+
+//
+// Region-based approach: transformations applied horizontally, for each Region
+// BB
+// +----+
+// |Rgn1| -> Transform1 -> ... -> TransformN -> Check Cost
+// | |
+// |Rgn2| -> Transform1 -> ... -> TransformN -> Check Cost
+// | |
+// |Rgn3| -> Transform1 -> ... -> TransformN -> Check Cost
+// +----+
+
+class Region {
+ /// All the instructions in the Region. Only new instructions generated during
+ /// vectorization are part of the Region.
+ SetVector<sandboxir::Instruction *> Insts;
+
+ /// A unique ID, used for debugging.
+ unsigned RegionID = 0;
+
+ sandboxir::Context &Ctx;
+
+ /// The basic block containing this region.
+ sandboxir::BasicBlock &SBBB;
+
+ // TODO: Add cost modeling.
+
+ /// The cost of the original scalar instructions replaced by vector
+ /// instructions in this region.
+ InstructionCost ScalarCost;
+
+ /// The cost of the vector instructions in the region.
+ InstructionCost VectorCost;
+
+ // TODO: Add a way to encode/decode region info to/from metadata.
+
+public:
+ Region(sandboxir::Context &Ctx, sandboxir::BasicBlock &SBBB);
+ ~Region();
+
+ sandboxir::BasicBlock *getParent() const { return &SBBB; }
+ sandboxir::Context &getContext() const { return Ctx; }
+ /// Returns the region's unique ID.
+ unsigned getID() const { return RegionID; }
+
+ /// Adds SBI to the set.
+ void add(sandboxir::Instruction *SBI);
+ /// Removes SBI from the set.
+ void remove(sandboxir::Instruction *SBI);
+ /// Returns true if SBI is in the Region.
+ bool contains(sandboxir::Instruction *SBI) const {
+ return Insts.contains(SBI);
+ }
+ /// Returns true if the Region has no instructions.
+ bool empty() const { return Insts.empty(); }
+
+ using iterator = decltype(Insts.begin());
+ iterator begin() { return Insts.begin(); }
+ iterator end() { return Insts.end(); }
+
+#ifndef NDEBUG
+ /// This is an expensive check, meant for testing.
+ bool operator==(const sandboxir::Region &Other) const;
+ void dump(raw_ostream &OS) const;
+ void dump() const;
+ friend raw_ostream &operator<<(raw_ostream &OS,
+ const sandboxir::Region &Rgn) {
+ Rgn.dump(OS);
+ return OS;
+ }
+#endif
+};
+
+} // namespace sandboxir
+} // namespace llvm
+#endif // LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_REGION_H
diff --git a/llvm/lib/Transforms/Vectorize/CMakeLists.txt b/llvm/lib/Transforms/Vectorize/CMakeLists.txt
index 59d04ac3cecd00..f33906b05fedd1 100644
--- a/llvm/lib/Transforms/Vectorize/CMakeLists.txt
+++ b/llvm/lib/Transforms/Vectorize/CMakeLists.txt
@@ -5,6 +5,7 @@ add_llvm_component_library(LLVMVectorize
LoopVectorize.cpp
SandboxVectorizer/DependencyGraph.cpp
SandboxVectorizer/Passes/BottomUpVec.cpp
+ SandboxVectorizer/Region.cpp
SandboxVectorizer/SandboxVectorizer.cpp
SLPVectorizer.cpp
Vectorize.cpp
diff --git a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
new file mode 100644
index 00000000000000..91c0d71becd757
--- /dev/null
+++ b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
@@ -0,0 +1,52 @@
+//===- Region.cpp ---------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Transforms/Vectorize/SandboxVectorizer/Region.h"
+
+using namespace llvm;
+
+sandboxir::Region::Region(sandboxir::Context &Ctx, sandboxir::BasicBlock &SBBB)
+ : Ctx(Ctx), SBBB(SBBB) {
+ static unsigned StaticRegionID;
+ RegionID = StaticRegionID++;
+}
+
+sandboxir::Region::~Region() {}
+
+void sandboxir::Region::add(sandboxir::Instruction *SBI) {
+ Insts.insert(SBI);
+}
+
+void sandboxir::Region::remove(sandboxir::Instruction *SBI) {
+ Insts.remove(SBI);
+}
+
+#ifndef NDEBUG
+bool sandboxir::Region::operator==(const sandboxir::Region &Other) const {
+ if (Insts.size() != Other.Insts.size())
+ return false;
+ if (!std::is_permutation(Insts.begin(), Insts.end(),
+ Other.Insts.begin()))
+ return false;
+ return true;
+}
+
+void sandboxir::Region::dump(raw_ostream &OS) const {
+ OS << "RegionID: " << getID() << " ScalarCost=" << ScalarCost
+ << " VectorCost=" << VectorCost << "\n";
+ for (auto *I : Insts)
+ OS << *I << "\n";
+}
+
+void sandboxir::Region::dump() const {
+ dump(dbgs());
+ dbgs() << "\n";
+}
+
+#endif // NDEBUG
+
>From cca2be159f032054e8cba438ec01e88ed33f8d5e Mon Sep 17 00:00:00 2001
From: Jorge Gorbe Moya <jgorbe at google.com>
Date: Mon, 16 Sep 2024 16:17:28 -0700
Subject: [PATCH 2/5] clang-format
---
.../lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
index 91c0d71becd757..edfdc0185fa119 100644
--- a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
+++ b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
@@ -18,9 +18,7 @@ sandboxir::Region::Region(sandboxir::Context &Ctx, sandboxir::BasicBlock &SBBB)
sandboxir::Region::~Region() {}
-void sandboxir::Region::add(sandboxir::Instruction *SBI) {
- Insts.insert(SBI);
-}
+void sandboxir::Region::add(sandboxir::Instruction *SBI) { Insts.insert(SBI); }
void sandboxir::Region::remove(sandboxir::Instruction *SBI) {
Insts.remove(SBI);
@@ -30,8 +28,7 @@ void sandboxir::Region::remove(sandboxir::Instruction *SBI) {
bool sandboxir::Region::operator==(const sandboxir::Region &Other) const {
if (Insts.size() != Other.Insts.size())
return false;
- if (!std::is_permutation(Insts.begin(), Insts.end(),
- Other.Insts.begin()))
+ if (!std::is_permutation(Insts.begin(), Insts.end(), Other.Insts.begin()))
return false;
return true;
}
@@ -49,4 +46,3 @@ void sandboxir::Region::dump() const {
}
#endif // NDEBUG
-
>From d5c5d58fe2279064e1260c31ead47d9b13be6aa4 Mon Sep 17 00:00:00 2001
From: Jorge Gorbe Moya <jgorbe at google.com>
Date: Tue, 17 Sep 2024 14:00:06 -0700
Subject: [PATCH 3/5] Address review feedback.
- Add tests.
- Remove cost-related comment and unused member variables.
- Drop unneeded "sandboxir::" prefixes.
- Remove "SB" prefix from some variables.
- Simplify nested namespace.
- Added `insts()` method that returns a {begin(), end()} iterator_range.
---
.../Vectorize/SandboxVectorizer/Region.h | 54 ++++--------
.../Vectorize/SandboxVectorizer/Region.cpp | 13 ++-
.../SandboxVectorizer/CMakeLists.txt | 1 +
.../SandboxVectorizer/RegionTest.cpp | 83 +++++++++++++++++++
4 files changed, 107 insertions(+), 44 deletions(-)
create mode 100644 llvm/unittests/Transforms/Vectorize/SandboxVectorizer/RegionTest.cpp
diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h
index 06c432c2c7b839..45b43aa77f415b 100644
--- a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h
@@ -9,13 +9,13 @@
#ifndef LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_REGION_H
#define LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_REGION_H
+#include "llvm/ADT/iterator_range.h"
#include "llvm/ADT/SetVector.h"
#include "llvm/SandboxIR/SandboxIR.h"
#include "llvm/Support/InstructionCost.h"
#include "llvm/Support/raw_ostream.h"
-namespace llvm {
-namespace sandboxir {
+namespace llvm::sandboxir {
/// The main job of the Region is to point to new instructions generated by
/// vectorization passes. It is the unit that RegionPasses operate on with their
@@ -28,19 +28,6 @@ namespace sandboxir {
/// to check for profitability and decide whether we accept or rollback at a
/// region granularity, which is much better than doing this at the BB level.
///
-/// The region keeps track of the costs that correspond to that region. That is
-/// the original "scalar" cost and the new "vector" cost. The cost is updated
-/// automatically via SandboxIR callbacks:
-/// (i) Upon deletion: If the deleted instruction is not in the region, it is an
-/// original scalar, so its cost is added to the "scalar cost", otherwise it is
-/// a "vector" instruction created by the vectorizer so we subtract its cost
-/// from the "vector cost".
-/// (ii) Upon creation: New instructions are always considered "vector" so its
-/// cost is added to the "vector cost".
-/// This cost tracking allows us to start with transformations that may not be
-/// profitable if applied by themselves, but may end up being profitable after a
-/// series of transformations.
-///
// Traditional approach: transformations applied vertically for the whole BB
// BB
// +----+
@@ -69,38 +56,30 @@ class Region {
/// A unique ID, used for debugging.
unsigned RegionID = 0;
- sandboxir::Context &Ctx;
+ Context &Ctx;
/// The basic block containing this region.
- sandboxir::BasicBlock &SBBB;
+ BasicBlock &BB;
// TODO: Add cost modeling.
-
- /// The cost of the original scalar instructions replaced by vector
- /// instructions in this region.
- InstructionCost ScalarCost;
-
- /// The cost of the vector instructions in the region.
- InstructionCost VectorCost;
-
// TODO: Add a way to encode/decode region info to/from metadata.
public:
- Region(sandboxir::Context &Ctx, sandboxir::BasicBlock &SBBB);
+ Region(sandboxir::Context &Ctx, sandboxir::BasicBlock &BB);
~Region();
- sandboxir::BasicBlock *getParent() const { return &SBBB; }
- sandboxir::Context &getContext() const { return Ctx; }
+ BasicBlock *getParent() const { return &BB; }
+ Context &getContext() const { return Ctx; }
/// Returns the region's unique ID.
unsigned getID() const { return RegionID; }
- /// Adds SBI to the set.
- void add(sandboxir::Instruction *SBI);
- /// Removes SBI from the set.
- void remove(sandboxir::Instruction *SBI);
- /// Returns true if SBI is in the Region.
- bool contains(sandboxir::Instruction *SBI) const {
- return Insts.contains(SBI);
+ /// Adds I to the set.
+ void add(sandboxir::Instruction *I);
+ /// Removes I from the set.
+ void remove(sandboxir::Instruction *I);
+ /// Returns true if I is in the Region.
+ bool contains(sandboxir::Instruction *I) const {
+ return Insts.contains(I);
}
/// Returns true if the Region has no instructions.
bool empty() const { return Insts.empty(); }
@@ -108,6 +87,7 @@ class Region {
using iterator = decltype(Insts.begin());
iterator begin() { return Insts.begin(); }
iterator end() { return Insts.end(); }
+ iterator_range<iterator> insts() { return make_range(begin(), end()); }
#ifndef NDEBUG
/// This is an expensive check, meant for testing.
@@ -122,6 +102,6 @@ class Region {
#endif
};
-} // namespace sandboxir
-} // namespace llvm
+} // namespace llvm::sandboxir
+
#endif // LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_REGION_H
diff --git a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
index edfdc0185fa119..d70a979b788cb9 100644
--- a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
+++ b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
@@ -10,18 +10,18 @@
using namespace llvm;
-sandboxir::Region::Region(sandboxir::Context &Ctx, sandboxir::BasicBlock &SBBB)
- : Ctx(Ctx), SBBB(SBBB) {
+sandboxir::Region::Region(sandboxir::Context &Ctx, sandboxir::BasicBlock &BB)
+ : Ctx(Ctx), BB(BB) {
static unsigned StaticRegionID;
RegionID = StaticRegionID++;
}
sandboxir::Region::~Region() {}
-void sandboxir::Region::add(sandboxir::Instruction *SBI) { Insts.insert(SBI); }
+void sandboxir::Region::add(sandboxir::Instruction *I) { Insts.insert(I); }
-void sandboxir::Region::remove(sandboxir::Instruction *SBI) {
- Insts.remove(SBI);
+void sandboxir::Region::remove(sandboxir::Instruction *I) {
+ Insts.remove(I);
}
#ifndef NDEBUG
@@ -34,8 +34,7 @@ bool sandboxir::Region::operator==(const sandboxir::Region &Other) const {
}
void sandboxir::Region::dump(raw_ostream &OS) const {
- OS << "RegionID: " << getID() << " ScalarCost=" << ScalarCost
- << " VectorCost=" << VectorCost << "\n";
+ OS << "RegionID: " << getID() << "\n";
for (auto *I : Insts)
OS << *I << "\n";
}
diff --git a/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/CMakeLists.txt b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/CMakeLists.txt
index 488c9c2344b56c..23dbe7f46bc995 100644
--- a/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/CMakeLists.txt
+++ b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/CMakeLists.txt
@@ -9,4 +9,5 @@ set(LLVM_LINK_COMPONENTS
add_llvm_unittest(SandboxVectorizerTests
DependencyGraphTest.cpp
+ RegionTest.cpp
)
diff --git a/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/RegionTest.cpp b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/RegionTest.cpp
new file mode 100644
index 00000000000000..37fa08212c1652
--- /dev/null
+++ b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/RegionTest.cpp
@@ -0,0 +1,83 @@
+//===- RegionTest.cpp -----------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Transforms/Vectorize/SandboxVectorizer/Region.h"
+#include "llvm/AsmParser/Parser.h"
+#include "llvm/SandboxIR/SandboxIR.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock-matchers.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+struct RegionTest : public testing::Test {
+ LLVMContext C;
+ std::unique_ptr<Module> M;
+
+ void parseIR(LLVMContext &C, const char *IR) {
+ SMDiagnostic Err;
+ M = parseAssemblyString(IR, Err, C);
+ if (!M)
+ Err.print("RegionTest", errs());
+ }
+};
+
+TEST_F(RegionTest, Basic) {
+ parseIR(C, R"IR(
+define i8 @foo(i8 %v0, i8 %v1) {
+ %t0 = add i8 %v0, 1
+ %t1 = add i8 %t0, %v1
+ ret i8 %t1
+}
+)IR");
+ llvm::Function *LLVMF = &*M->getFunction("foo");
+ sandboxir::Context Ctx(C);
+ auto *F = Ctx.createFunction(LLVMF);
+ auto *BB = &*F->begin();
+ auto It = BB->begin();
+ auto *T0 = cast<sandboxir::Instruction>(&*It++);
+ auto *T1 = cast<sandboxir::Instruction>(&*It++);
+ auto *Ret = cast<sandboxir::Instruction>(&*It++);
+ sandboxir::Region Rgn(Ctx, *BB);
+
+ // Check getters
+ EXPECT_EQ(BB, Rgn.getParent());
+ EXPECT_EQ(&Ctx, &Rgn.getContext());
+ EXPECT_EQ(0U, Rgn.getID());
+
+ // Check add / remove / empty.
+ EXPECT_TRUE(Rgn.empty());
+ Rgn.add(T0);
+ EXPECT_FALSE(Rgn.empty());
+ Rgn.remove(T0);
+ EXPECT_TRUE(Rgn.empty());
+
+ // Check iteration.
+ Rgn.add(T0);
+ Rgn.add(T1);
+ Rgn.add(Ret);
+ // Use an ordered matcher because we're supposed to preserve the insertion
+ // order for determinism.
+ EXPECT_THAT(Rgn.insts(), testing::ElementsAre(T0, T1, Ret));
+
+ // Check contains
+ EXPECT_TRUE(Rgn.contains(T0));
+ Rgn.remove(T0);
+ EXPECT_FALSE(Rgn.contains(T0));
+
+#ifndef NDEBUG
+ // Check equality comparison. Insert in reverse order into `Other` to check
+ // that comparison is order-independent.
+ sandboxir::Region Other(Ctx, *BB);
+ Other.add(Ret);
+ Other.add(T1);
+ EXPECT_EQ(Rgn, Other);
+#endif
+
+}
+
>From b3818914d36f771ae75d8c8a6c5a8fc18b790c61 Mon Sep 17 00:00:00 2001
From: Jorge Gorbe Moya <jgorbe at google.com>
Date: Tue, 17 Sep 2024 14:11:45 -0700
Subject: [PATCH 4/5] return of the clang-format
---
.../llvm/Transforms/Vectorize/SandboxVectorizer/Region.h | 6 ++----
llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp | 4 +---
.../Transforms/Vectorize/SandboxVectorizer/RegionTest.cpp | 2 --
3 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h
index 45b43aa77f415b..ccd43ffb3535b7 100644
--- a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h
@@ -9,8 +9,8 @@
#ifndef LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_REGION_H
#define LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_REGION_H
-#include "llvm/ADT/iterator_range.h"
#include "llvm/ADT/SetVector.h"
+#include "llvm/ADT/iterator_range.h"
#include "llvm/SandboxIR/SandboxIR.h"
#include "llvm/Support/InstructionCost.h"
#include "llvm/Support/raw_ostream.h"
@@ -78,9 +78,7 @@ class Region {
/// Removes I from the set.
void remove(sandboxir::Instruction *I);
/// Returns true if I is in the Region.
- bool contains(sandboxir::Instruction *I) const {
- return Insts.contains(I);
- }
+ bool contains(sandboxir::Instruction *I) const { return Insts.contains(I); }
/// Returns true if the Region has no instructions.
bool empty() const { return Insts.empty(); }
diff --git a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
index d70a979b788cb9..772222ccb0fdec 100644
--- a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
+++ b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
@@ -20,9 +20,7 @@ sandboxir::Region::~Region() {}
void sandboxir::Region::add(sandboxir::Instruction *I) { Insts.insert(I); }
-void sandboxir::Region::remove(sandboxir::Instruction *I) {
- Insts.remove(I);
-}
+void sandboxir::Region::remove(sandboxir::Instruction *I) { Insts.remove(I); }
#ifndef NDEBUG
bool sandboxir::Region::operator==(const sandboxir::Region &Other) const {
diff --git a/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/RegionTest.cpp b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/RegionTest.cpp
index 37fa08212c1652..aa961c0092e56c 100644
--- a/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/RegionTest.cpp
+++ b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/RegionTest.cpp
@@ -78,6 +78,4 @@ define i8 @foo(i8 %v0, i8 %v1) {
Other.add(T1);
EXPECT_EQ(Rgn, Other);
#endif
-
}
-
>From b459cf0320081e87ff609e2e6c8ded90373dc9cb Mon Sep 17 00:00:00 2001
From: Jorge Gorbe Moya <jgorbe at google.com>
Date: Tue, 17 Sep 2024 14:37:27 -0700
Subject: [PATCH 5/5] Another round of review feedback
- Removed the rest of unneeded `sandboxir::` prefixes. Also moved the
code in Region.cpp to the namespace llvm::sandboxir.
- Added `operator!=` to go with the existing `operator==`, used it in
the test, as requested.
---
.../Vectorize/SandboxVectorizer/Region.h | 17 +++++++++--------
.../Vectorize/SandboxVectorizer/Region.cpp | 19 ++++++++++---------
.../SandboxVectorizer/RegionTest.cpp | 1 +
3 files changed, 20 insertions(+), 17 deletions(-)
diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h
index ccd43ffb3535b7..71d9ab58250993 100644
--- a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Region.h
@@ -51,7 +51,7 @@ namespace llvm::sandboxir {
class Region {
/// All the instructions in the Region. Only new instructions generated during
/// vectorization are part of the Region.
- SetVector<sandboxir::Instruction *> Insts;
+ SetVector<Instruction *> Insts;
/// A unique ID, used for debugging.
unsigned RegionID = 0;
@@ -65,7 +65,7 @@ class Region {
// TODO: Add a way to encode/decode region info to/from metadata.
public:
- Region(sandboxir::Context &Ctx, sandboxir::BasicBlock &BB);
+ Region(Context &Ctx, BasicBlock &BB);
~Region();
BasicBlock *getParent() const { return &BB; }
@@ -74,11 +74,11 @@ class Region {
unsigned getID() const { return RegionID; }
/// Adds I to the set.
- void add(sandboxir::Instruction *I);
+ void add(Instruction *I);
/// Removes I from the set.
- void remove(sandboxir::Instruction *I);
+ void remove(Instruction *I);
/// Returns true if I is in the Region.
- bool contains(sandboxir::Instruction *I) const { return Insts.contains(I); }
+ bool contains(Instruction *I) const { return Insts.contains(I); }
/// Returns true if the Region has no instructions.
bool empty() const { return Insts.empty(); }
@@ -89,11 +89,12 @@ class Region {
#ifndef NDEBUG
/// This is an expensive check, meant for testing.
- bool operator==(const sandboxir::Region &Other) const;
+ bool operator==(const Region &Other) const;
+ bool operator!=(const Region &other) const { return !(*this == other); }
+
void dump(raw_ostream &OS) const;
void dump() const;
- friend raw_ostream &operator<<(raw_ostream &OS,
- const sandboxir::Region &Rgn) {
+ friend raw_ostream &operator<<(raw_ostream &OS, const Region &Rgn) {
Rgn.dump(OS);
return OS;
}
diff --git a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
index 772222ccb0fdec..0c5d66cdeb8f40 100644
--- a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
+++ b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Region.cpp
@@ -8,22 +8,21 @@
#include "llvm/Transforms/Vectorize/SandboxVectorizer/Region.h"
-using namespace llvm;
+namespace llvm::sandboxir {
-sandboxir::Region::Region(sandboxir::Context &Ctx, sandboxir::BasicBlock &BB)
- : Ctx(Ctx), BB(BB) {
+Region::Region(Context &Ctx, BasicBlock &BB) : Ctx(Ctx), BB(BB) {
static unsigned StaticRegionID;
RegionID = StaticRegionID++;
}
-sandboxir::Region::~Region() {}
+Region::~Region() {}
-void sandboxir::Region::add(sandboxir::Instruction *I) { Insts.insert(I); }
+void Region::add(Instruction *I) { Insts.insert(I); }
-void sandboxir::Region::remove(sandboxir::Instruction *I) { Insts.remove(I); }
+void Region::remove(Instruction *I) { Insts.remove(I); }
#ifndef NDEBUG
-bool sandboxir::Region::operator==(const sandboxir::Region &Other) const {
+bool Region::operator==(const Region &Other) const {
if (Insts.size() != Other.Insts.size())
return false;
if (!std::is_permutation(Insts.begin(), Insts.end(), Other.Insts.begin()))
@@ -31,15 +30,17 @@ bool sandboxir::Region::operator==(const sandboxir::Region &Other) const {
return true;
}
-void sandboxir::Region::dump(raw_ostream &OS) const {
+void Region::dump(raw_ostream &OS) const {
OS << "RegionID: " << getID() << "\n";
for (auto *I : Insts)
OS << *I << "\n";
}
-void sandboxir::Region::dump() const {
+void Region::dump() const {
dump(dbgs());
dbgs() << "\n";
}
+} // namespace llvm::sandboxir
+
#endif // NDEBUG
diff --git a/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/RegionTest.cpp b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/RegionTest.cpp
index aa961c0092e56c..85b567ed51cd8a 100644
--- a/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/RegionTest.cpp
+++ b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/RegionTest.cpp
@@ -75,6 +75,7 @@ define i8 @foo(i8 %v0, i8 %v1) {
// that comparison is order-independent.
sandboxir::Region Other(Ctx, *BB);
Other.add(Ret);
+ EXPECT_NE(Rgn, Other);
Other.add(T1);
EXPECT_EQ(Rgn, Other);
#endif
More information about the llvm-commits
mailing list