[SandboxVec] Implement Pass class #107617

vporpo · 2024-09-06T17:26:06Z

This patch implements the Pass base class and the FunctionPass sub-class that operate on Sandbox IR.

llvmbot · 2024-09-06T17:26:38Z

@llvm/pr-subscribers-llvm-transforms

Author: vporpo (vporpo)

Changes

This patch implements the Pass base class and the FunctionPass sub-class that operate on Sandbox IR.

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

6 Files Affected:

(added) llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Pass.h (+83)
(modified) llvm/lib/Transforms/Vectorize/CMakeLists.txt (+1)
(added) llvm/lib/Transforms/Vectorize/SandboxVectorizer/Pass.cpp (+19)
(modified) llvm/unittests/CMakeLists.txt (+1)
(added) llvm/unittests/SandboxVectorizer/CMakeLists.txt (+9)
(added) llvm/unittests/SandboxVectorizer/PassTest.cpp (+67)

diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Pass.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Pass.h
new file mode 100644
index 00000000000000..fa45e4808c0f6f
--- /dev/null
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Pass.h
@@ -0,0 +1,83 @@
+//===- Pass.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_PASS_H
+#define LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASS_H
+
+#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/raw_ostream.h"
+#include <string>
+
+namespace llvm::sandboxir {
+
+class Function;
+
+/// The base class of a Sandbox IR Pass.
+class Pass {
+public:
+  enum class ClassID : unsigned {
+    FunctionPass,
+  };
+  static const char *getSubclassIDStr(ClassID ID) {
+    switch (ID) {
+    case ClassID::FunctionPass:
+      return "FunctionPass";
+    }
+    llvm_unreachable("Unimplemented ID");
+  }
+
+protected:
+  /// The pass name.
+  const std::string Name;
+  /// The command-line flag used to specify that this pass should run.
+  const std::string Flag;
+  /// Used for isa/cast/dyn_cast.
+  ClassID SubclassID;
+
+public:
+  Pass(const std::string &Name, const std::string &Flag, ClassID SubclassID)
+      : Name(Name), Flag(Flag), SubclassID(SubclassID) {}
+  virtual ~Pass() {}
+  /// \Returns the name of the pass.
+  StringRef getName() const { return Name; }
+  /// \Returns the command-line flag used to enable the pass.
+  StringRef getFlag() const { return Flag; }
+  ClassID getSubclassID() const { return SubclassID; }
+#ifndef NDEBUG
+  friend raw_ostream &operator<<(raw_ostream &OS, const Pass &Pass) {
+    Pass.dump(OS);
+    return OS;
+  }
+  void dump(raw_ostream &OS) const { OS << Name << " " << Flag; }
+  LLVM_DUMP_METHOD void dump() const;
+#endif
+};
+
+/// A pass that runs on a sandbox::Function.
+class FunctionPass : public Pass {
+protected:
+  FunctionPass(const std::string &Name, const std::string &Flag, ClassID PassID)
+      : Pass(Name, Flag, PassID) {}
+
+public:
+  FunctionPass(const std::string &Name, const std::string &Flag)
+      : Pass(Name, Flag, ClassID::FunctionPass) {}
+  /// For isa/dyn_cast etc.
+  static bool classof(const Pass *From) {
+    switch (From->getSubclassID()) {
+    case ClassID::FunctionPass:
+      return true;
+    }
+  }
+  /// \Returns true if it modifies \p F.
+  virtual bool runOnFunction(Function &F) = 0;
+};
+
+} // namespace llvm::sandboxir
+
+#endif // LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASS_H
diff --git a/llvm/lib/Transforms/Vectorize/CMakeLists.txt b/llvm/lib/Transforms/Vectorize/CMakeLists.txt
index 649faad48d71e5..2406856d4eaf09 100644
--- a/llvm/lib/Transforms/Vectorize/CMakeLists.txt
+++ b/llvm/lib/Transforms/Vectorize/CMakeLists.txt
@@ -3,6 +3,7 @@ add_llvm_component_library(LLVMVectorize
   LoopIdiomVectorize.cpp
   LoopVectorizationLegality.cpp
   LoopVectorize.cpp
+  SandboxVectorizer/Pass.cpp
   SandboxVectorizer/SandboxVectorizer.cpp
   SLPVectorizer.cpp
   Vectorize.cpp
diff --git a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Pass.cpp b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Pass.cpp
new file mode 100644
index 00000000000000..686c75af5586fd
--- /dev/null
+++ b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/Pass.cpp
@@ -0,0 +1,19 @@
+//===- Pass.cpp - Passes that operate on Sandbox IR -----------------------===//
+//
+// 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/Pass.h"
+#include "llvm/Support/Debug.h"
+
+using namespace llvm::sandboxir;
+
+#ifndef NDEBUG
+void Pass::dump() const {
+  dump(dbgs());
+  dbgs() << "\n";
+}
+#endif // NDEBUG
diff --git a/llvm/unittests/CMakeLists.txt b/llvm/unittests/CMakeLists.txt
index 911ede701982f6..e709cf161cf6ad 100644
--- a/llvm/unittests/CMakeLists.txt
+++ b/llvm/unittests/CMakeLists.txt
@@ -45,6 +45,7 @@ add_subdirectory(Remarks)
 add_subdirectory(Passes)
 add_subdirectory(ProfileData)
 add_subdirectory(SandboxIR)
+add_subdirectory(SandboxVectorizer)
 add_subdirectory(Support)
 add_subdirectory(TableGen)
 add_subdirectory(Target)
diff --git a/llvm/unittests/SandboxVectorizer/CMakeLists.txt b/llvm/unittests/SandboxVectorizer/CMakeLists.txt
new file mode 100644
index 00000000000000..6c32364ff412d6
--- /dev/null
+++ b/llvm/unittests/SandboxVectorizer/CMakeLists.txt
@@ -0,0 +1,9 @@
+set(LLVM_LINK_COMPONENTS
+  AsmParser
+  SandboxIR
+  Core
+  )
+
+add_llvm_unittest(SandboxVectorizerTests
+  PassTest.cpp
+  )
diff --git a/llvm/unittests/SandboxVectorizer/PassTest.cpp b/llvm/unittests/SandboxVectorizer/PassTest.cpp
new file mode 100644
index 00000000000000..7dff8830bd7bb5
--- /dev/null
+++ b/llvm/unittests/SandboxVectorizer/PassTest.cpp
@@ -0,0 +1,67 @@
+//===- PassesTest.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/Pass.h"
+#include "llvm/AsmParser/Parser.h"
+#include "llvm/IR/Module.h"
+#include "llvm/SandboxIR/SandboxIR.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gtest/gtest.h"
+
+using namespace llvm::sandboxir;
+
+struct PassTest : public testing::Test {
+  llvm::LLVMContext LLVMCtx;
+  std::unique_ptr<llvm::Module> LLVMM;
+  std::unique_ptr<Context> Ctx;
+
+  Function *parseFunction(const char *IR, const char *FuncName) {
+    llvm::SMDiagnostic Err;
+    LLVMM = parseAssemblyString(IR, Err, LLVMCtx);
+    if (!LLVMM)
+      Err.print("PassTest", llvm::errs());
+    Ctx = std::make_unique<Context>(LLVMCtx);
+    return Ctx->createFunction(LLVMM->getFunction(FuncName));
+  }
+};
+
+TEST_F(PassTest, FunctionPass) {
+  auto *F = parseFunction(R"IR(
+define void @foo() {
+  ret void
+}
+)IR",
+                          "foo");
+  class TestPass final : public FunctionPass {
+    unsigned &BBCnt;
+
+  public:
+    TestPass(unsigned &BBCnt)
+        : FunctionPass("TestPass", "-test-pass"), BBCnt(BBCnt) {}
+    bool runOnFunction(Function &F) final {
+      for ([[maybe_unused]] auto &BB : F)
+        ++BBCnt;
+      return false;
+    }
+  };
+  unsigned BBCnt = 0;
+  TestPass TPass(BBCnt);
+  // Check getName(),
+  EXPECT_EQ(TPass.getName(), "TestPass");
+  // Check getFlag().
+  EXPECT_EQ(TPass.getFlag(), "-test-pass");
+  // Check getSubclassID().
+  EXPECT_EQ(TPass.getSubclassID(), Pass::ClassID::FunctionPass);
+  // Check getSubclassIDStr().
+  EXPECT_EQ(Pass::getSubclassIDStr(TPass.getSubclassID()), "FunctionPass");
+  // Check classof().
+  EXPECT_TRUE(llvm::isa<FunctionPass>(TPass));
+  // Check runOnFunction();
+  TPass.runOnFunction(*F);
+  EXPECT_EQ(BBCnt, 1u);
+}

tschuett · 2024-09-06T17:32:30Z

Are they vectoriser specific? Alternative place them here:
https://github.com/llvm/llvm-project/tree/main/llvm/include/llvm/SandboxIR
?

tschuett · 2024-09-06T17:33:58Z

llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Pass.h

+  ClassID SubclassID;
+
+public:
+  Pass(const std::string &Name, const std::string &Flag, ClassID SubclassID)


I believe that we moved from const std::string & to llvm::StringRef .

tschuett · 2024-09-06T17:35:25Z

llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Pass.h

+  /// The pass name.
+  const std::string Name;
+  /// The command-line flag used to specify that this pass should run.
+  const std::string Flag;


Or SmallString.

The user could give arbitrarily large names/flags so isn't an std::string better? I mean, what size should I use?

It is a wrapper around a SmallVector. It is just stack friendlier than std::string.

You can just leave it as std::string.

vporpo · 2024-09-06T17:37:53Z

Are they vectoriser specific?

They are not specific to the vectorizer.

Yeah llvm/SandboxIR seems like a better place.

tschuett · 2024-09-06T18:06:32Z

llvm/include/llvm/SandboxIR/Pass.h

+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASS_H
+#define LLVM_TRANSFORMS_VECTORIZE_SANDBOXVECTORIZER_PASS_H


Still the old name.

Good catch :)

aeubanks · 2024-09-09T15:06:56Z

llvm/include/llvm/SandboxIR/Pass.h

+    Pass.dumpOS(OS);
+    return OS;
+  }
+  void dumpOS(raw_ostream &OS) const { OS << Name << " " << Flag; }


I think this is usually named print

aeubanks · 2024-09-09T15:10:13Z

llvm/include/llvm/SandboxIR/Pass.h

+  FunctionPass(StringRef Name, StringRef Flag)
+      : Pass(Name, Flag, ClassID::FunctionPass) {}
+  /// For isa/dyn_cast etc.
+  static bool classof(const Pass *From) {


do we need isa/dyn_cast support? can we just add function/region/block passes to a pass manager that only holds that type? following the new pass manager pass adaptor/manager model

I think that would complicate things. I was thinking of a pass manager that hods Pass * objects.

the thing about the legacy and new pass managers is that they run a bunch of function passes on one function at a time, then move to the next. they don't just run one function pass on every function then move on to the next. this is important for data locality for compile times, although perhaps that's not as important here. it's also fits in with the call graph walk where you want to see simplified callees

if you want the above, you need some sort of scheduling, whether dynamically (legacy pass manager) or statically in the code via adaptors and managers (new pass manager)

if you don't want the above, then you shouldn't need isa/dyn_cast, just have subclasses implement a virtual run()

Our pass manager will operate in a similar way: The top-level pass manager will visit the functions in some order and for each one of them it will call runOnFunction() for all registered passes before moving to the next function. The pass order is fixed. One or more function passes will also be a Region pass manager and that will operate in a similar way: it will go over the regions and for each one of them will call runOnRegion() for all registered region passes.

We could use a virtual run() but if we want to keep the runOnFunction/Region() names I think we need the isa/dyn_cast stuff.

I modified the PassManager's code a bit and I think we can remove the casts, as you suggested. Each pass manager will contain a vector of passes of the subclass type.

vporpo · 2024-09-09T16:58:02Z

llvm/include/llvm/SandboxIR/Pass.h

+  /// The pass name.
+  const std::string Name;
+  /// The command-line flag used to specify that this pass should run.
+  const std::string Flag;


I think I will drop the Flag. Even though Name could be used more of a description of the pass, I think we should simplify things for now and just use Name for the command-line flag too.

aeubanks

didn't notice the first time, but I would have thought this would go in the SandboxVectorizer, not SandboxIR? why are you deciding to put this in SandboxIR?

aeubanks · 2024-09-09T22:20:27Z

llvm/unittests/SandboxIR/PassTest.cpp

@@ -0,0 +1,84 @@
+//===- PassesTest.cpp -----------------------------------------------------===//


nit: file name

aeubanks

(otherwise this lgtm)

This patch implements the Pass base class and the FunctionPass sub-class that operate on Sandbox IR.

vporpo · 2024-09-09T22:36:05Z

didn't notice the first time, but I would have thought this would go in the SandboxVectorizer, not SandboxIR? why are you deciding to put this in SandboxIR?

It was initially in SandboxVectorizer but the Pass and FunctionPass classes are not vectorizer specific so @tschuett suggested we move it to SandboxIR, which sounds reasonable. The Region that the vectorizer will use is specific to the vectorizer, so we can implement that within the SandboxVectorizer directory.

aeubanks · 2024-09-09T22:43:40Z

oh I should have looked at the previous comments

this doesn't really feel like part of SandboxIR but a layer on top that's fairly specific to the vectorizer (I'm guessing most LLVM IR passes that use SandboxIR probably aren't going to have sub-passes that use this pass infra). I would have kept it in SandboxVectorizer, but oh well

vporpo · 2024-09-09T22:50:31Z

this doesn't really feel like part of SandboxIR but a layer on top that's fairly specific to the vectorizer (I'm guessing most LLVM IR passes that use SandboxIR probably aren't going to have sub-passes that use this pass infra). I would have kept it in SandboxVectorizer, but oh well

Ideally I think these files should be in a top-level directory like llvm/SandboxIRPass. Yeah I doubt there will be other users of these passes, but separating the vectorizer-specific stuff from the rest is a good idea in general.

llvm-ci · 2024-09-10T09:15:29Z

LLVM Buildbot has detected a new failure on builder lld-x86_64-win running on as-worker-93 while building llvm at step 7 "test-build-unified-tree-check-all".

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

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

Step 7 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests.exe/43/86' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe-LLVM-Unit-14416-43-86.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=86 GTEST_SHARD_INDEX=43 C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe
--

Script:
--
C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe --gtest_filter=ProgramEnvTest.CreateProcessLongPath
--
C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(160): error: Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(163): error: fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied



C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:160
Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:163
fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied




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

vporpo requested review from slackito, alinas, tschuett, aeubanks and Sterling-Augustine September 6, 2024 17:26

llvmbot added vectorizers llvm:transforms labels Sep 6, 2024

tschuett reviewed Sep 6, 2024

View reviewed changes

vporpo force-pushed the vec branch from e78236c to 4623e12 Compare September 6, 2024 18:01

tschuett reviewed Sep 6, 2024

View reviewed changes

vporpo force-pushed the vec branch from 1bc221a to 0f1bce7 Compare September 6, 2024 22:22

aeubanks reviewed Sep 9, 2024

View reviewed changes

vporpo commented Sep 9, 2024

View reviewed changes

aeubanks reviewed Sep 9, 2024

View reviewed changes

aeubanks approved these changes Sep 9, 2024

View reviewed changes

[SandboxVec] Implement Pass class

91b9f9a

This patch implements the Pass base class and the FunctionPass sub-class that operate on Sandbox IR.

vporpo force-pushed the vec branch from fae107d to 91b9f9a Compare September 9, 2024 22:29

vporpo merged commit f12e10b into llvm:main Sep 9, 2024
5 of 7 checks passed

		@@ -0,0 +1,84 @@
		//===- PassesTest.cpp -----------------------------------------------------===//

[SandboxVec] Implement Pass class #107617

[SandboxVec] Implement Pass class #107617

Uh oh!

Conversation

vporpo commented Sep 6, 2024

Uh oh!

llvmbot commented Sep 6, 2024

Uh oh!

tschuett commented Sep 6, 2024

Uh oh!

tschuett Sep 6, 2024 • 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!

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!

vporpo commented Sep 6, 2024

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!

aeubanks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeubanks left a comment

Choose a reason for hiding this comment

Uh oh!

vporpo commented Sep 9, 2024

Uh oh!

Uh oh!

aeubanks commented Sep 9, 2024

Uh oh!

vporpo commented Sep 9, 2024

Uh oh!

llvm-ci commented Sep 10, 2024

Uh oh!

Uh oh!

tschuett Sep 6, 2024 •

edited

Loading