[SandboxVec] Clear Context's state within runOnFunction() #124842
Merged
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
`sandboxir::Context` is defined at a pass-level scope with the `SandboxVectorizerPass` class because the function pass manager `FPM` object depends on it, and that is in pass-level scope to avoid recreating the pass pipeline every single time `runOnFunction()` is called. This means that the Context's state lives on across function passes. The problem is twofold: (i) the LLVM IR to Sandbox IR map can grow very large including objects from different functions, which is of no use to the vectorizer, as it's a function-level pass. (ii) this can result in stale data in the LLVM IR to Sandbox IR object map, as other passes may delete LLVM IR objects. To fix both issues this patch introduces a `Context::clear()` function that clears the `LLVMValueToValueMap`.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-vectorizers Author: vporpo (vporpo) Changes
This means that the Context's state lives on across function passes. The problem is twofold: To fix both issues this patch introduces a Full diff: https://github.com/llvm/llvm-project/pull/124842.diff 6 Files Affected:
diff --git a/llvm/include/llvm/SandboxIR/Context.h b/llvm/include/llvm/SandboxIR/Context.h
index 7fe97d984b9589..a88b0003f55bdb 100644
--- a/llvm/include/llvm/SandboxIR/Context.h
+++ b/llvm/include/llvm/SandboxIR/Context.h
@@ -218,6 +218,8 @@ class Context {
public:
Context(LLVMContext &LLVMCtx);
~Context();
+ /// Clears function-level state.
+ void clear();
Tracker &getTracker() { return IRTracker; }
/// Convenience function for `getTracker().save()`
diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.h
index 09369dbb496fce..7ea9386f08beef 100644
--- a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.h
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.h
@@ -24,10 +24,18 @@ class SandboxVectorizerPass : public PassInfoMixin<SandboxVectorizerPass> {
TargetTransformInfo *TTI = nullptr;
AAResults *AA = nullptr;
ScalarEvolution *SE = nullptr;
-
+ // NOTE: We define the Context as a pass-scope object instead of local object
+ // in runOnFunction() because the passes defined in the pass-manager need
+ // access to it for registering/deregistering callbacks during construction
+ // and destruction.
std::unique_ptr<sandboxir::Context> Ctx;
// A pipeline of SandboxIR function passes run by the vectorizer.
+ // NOTE: We define this as a pass-scope object to avoid recreating the
+ // pass-pipeline every time in runOnFunction(). The downside is that the
+ // Context also needs to be defined as a pass-scope object because the passes
+ // within FPM may register/unregister callbacks, so they need access to
+ // Context.
sandboxir::FunctionPassManager FPM;
bool runImpl(Function &F);
diff --git a/llvm/lib/SandboxIR/Context.cpp b/llvm/lib/SandboxIR/Context.cpp
index 440210f5a1bf7b..830f2832853fe5 100644
--- a/llvm/lib/SandboxIR/Context.cpp
+++ b/llvm/lib/SandboxIR/Context.cpp
@@ -611,6 +611,12 @@ Context::Context(LLVMContext &LLVMCtx)
Context::~Context() {}
+void Context::clear() {
+ // TODO: Ideally we should clear only function-scope objects, and keep global
+ // objects, like Constants to avoid recreating them.
+ LLVMValueToValueMap.clear();
+}
+
Module *Context::getModule(llvm::Module *LLVMM) const {
auto It = LLVMModuleToModuleMap.find(LLVMM);
if (It != LLVMModuleToModuleMap.end())
diff --git a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.cpp
index 542fcde71e83c6..798a0ad915375b 100644
--- a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizer.cpp
@@ -88,7 +88,10 @@ bool SandboxVectorizerPass::runImpl(Function &LLVMF) {
sandboxir::Function &F = *Ctx->createFunction(&LLVMF);
sandboxir::Analyses A(*AA, *SE, *TTI);
bool Change = FPM.runOnFunction(F, A);
- // TODO: This is a function pass, so we won't be needing the function-level
- // Sandbox IR objects in the future. So we should clear them.
+ // Given that sandboxir::Context `Ctx` is defined at a pass-level scope, the
+ // maps from LLVM IR to Sandbox IR may go stale as later passes remove LLVM IR
+ // objects. To avoid issues caused by this clear the context's state.
+ // NOTE: The alternative would be to define Ctx and FPM within runOnFunction()
+ Ctx->clear();
return Change;
}
diff --git a/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/CMakeLists.txt b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/CMakeLists.txt
index bbfbcc730a4cbe..104a27975cfc0c 100644
--- a/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/CMakeLists.txt
+++ b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/CMakeLists.txt
@@ -12,6 +12,7 @@ add_llvm_unittest(SandboxVectorizerTests
InstrMapsTest.cpp
IntervalTest.cpp
LegalityTest.cpp
+ SandboxVectorizerTest.cpp
SchedulerTest.cpp
SeedCollectorTest.cpp
VecUtilsTest.cpp
diff --git a/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizerTest.cpp b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizerTest.cpp
new file mode 100644
index 00000000000000..e9b304618a06ca
--- /dev/null
+++ b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/SandboxVectorizerTest.cpp
@@ -0,0 +1,63 @@
+//===- SandboxVectorizerTest.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/SandboxVectorizer.h"
+#include "llvm/Analysis/AssumptionCache.h"
+#include "llvm/Analysis/BasicAliasAnalysis.h"
+#include "llvm/Analysis/LoopInfo.h"
+#include "llvm/Analysis/ScalarEvolution.h"
+#include "llvm/Analysis/TargetLibraryInfo.h"
+#include "llvm/Analysis/TargetTransformInfo.h"
+#include "llvm/AsmParser/Parser.h"
+#include "llvm/IR/DataLayout.h"
+#include "llvm/IR/Dominators.h"
+#include "llvm/IR/PassInstrumentation.h"
+#include "llvm/SandboxIR/Function.h"
+#include "llvm/SandboxIR/Instruction.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+struct SandboxVectorizerTest : 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("SandboxVectorizerTest", errs());
+ }
+};
+
+// Check that we can run the pass on the same function more than once without
+// issues. This basically checks that Sandbox IR Context gets cleared after we
+// run the function pass.
+TEST_F(SandboxVectorizerTest, ContextCleared) {
+ parseIR(C, R"IR(
+define void @foo() {
+ ret void
+}
+)IR");
+ auto &LLVMF = *M->getFunction("foo");
+ SandboxVectorizerPass SVecPass;
+ FunctionAnalysisManager AM;
+ AM.registerPass([] { return TargetIRAnalysis(); });
+ AM.registerPass([] { return AAManager(); });
+ AM.registerPass([] { return ScalarEvolutionAnalysis(); });
+ AM.registerPass([] { return PassInstrumentationAnalysis(); });
+ AM.registerPass([] { return TargetLibraryAnalysis(); });
+ AM.registerPass([] { return AssumptionAnalysis(); });
+ AM.registerPass([] { return DominatorTreeAnalysis(); });
+ AM.registerPass([] { return LoopAnalysis(); });
+ SVecPass.run(LLVMF, AM);
+ // This shouldn't crash.
+ SVecPass.run(LLVMF, AM);
+}
|
slackito
approved these changes
Jan 29, 2025
| AM.registerPass([] { return LoopAnalysis(); }); | ||
| SVecPass.run(LLVMF, AM); | ||
| // This shouldn't crash. | ||
| SVecPass.run(LLVMF, AM); |
There was a problem hiding this comment.
Would it be possible to also test the case where some llvm::Value is deleted and you try to get a sandboxir::Value from the Context by using the pointer to the deleted value? (and check you don't get a sandboxir::Value back, of course)
There was a problem hiding this comment.
The problem is that I don't have access to sandboxir::Context at this level. The context is created and destroyed within the SVecPass. Any thoughts on how we could test what you are suggesting?
There was a problem hiding this comment.
I will push this patch to get rid of the bug. We can follow up with a follow-up patch if needed.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/2195 Here is the relevant piece of the build log for the reference |
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.
sandboxir::Contextis defined at a pass-level scope with theSandboxVectorizerPassclass because the function pass managerFPMobject depends on it, and that is in pass-level scope to avoid recreating the pass pipeline every single timerunOnFunction()is called.This means that the Context's state lives on across function passes. The problem is twofold:
(i) the LLVM IR to Sandbox IR map can grow very large including objects from different functions, which is of no use to the vectorizer, as it's a function-level pass.
(ii) this can result in stale data in the LLVM IR to Sandbox IR object map, as other passes may delete LLVM IR objects.
To fix both issues this patch introduces a
Context::clear()function that clears theLLVMValueToValueMap.