aeubanks updated this revision to Diff 505169.
aeubanks retitled this revision from "[StandardInstrumentations] Check that the
number of instructions doesn't change if analyses are preserved" to
"[StandardInstrumentations] Verify function doesn't change if analyses are
preserved".
aeubanks edited the summary of this revision.
aeubanks added a comment.
split out other changes, use StructuralHash instead
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146003/new/
https://reviews.llvm.org/D146003
Files:
llvm/include/llvm/IR/StructuralHash.h
llvm/lib/IR/StructuralHash.cpp
llvm/lib/Passes/StandardInstrumentations.cpp
llvm/unittests/IR/PassManagerTest.cpp
Index: llvm/unittests/IR/PassManagerTest.cpp
===================================================================
--- llvm/unittests/IR/PassManagerTest.cpp
+++ llvm/unittests/IR/PassManagerTest.cpp
@@ -950,4 +950,37 @@
FPM.addPass(TestSimplifyCFGWrapperPass(InnerFPM));
FPM.run(*F, FAM);
}
+
+#ifdef EXPENSIVE_CHECKS
+
+struct WrongFunctionPass : PassInfoMixin<WrongFunctionPass> {
+ PreservedAnalyses run(Function &F, FunctionAnalysisManager &FAM) {
+ F.getEntryBlock().begin()->eraseFromParent();
+ return PreservedAnalyses::all();
+ }
+ static StringRef name() { return "WrongFunctionPass"; }
+};
+
+TEST_F(PassManagerTest, FunctionAnalysisMissedInvalidation) {
+ LLVMContext Context;
+ auto M = parseIR(Context, "define void @foo() {\n"
+ " %a = add i32 0, 0\n"
+ " ret void\n"
+ "}\n");
+
+ FunctionAnalysisManager FAM;
+ PassInstrumentationCallbacks PIC;
+ StandardInstrumentations SI(M->getContext(), /*DebugLogging*/ false);
+ SI.registerCallbacks(PIC, &FAM);
+ FAM.registerPass([&] { return PassInstrumentationAnalysis(&PIC); });
+
+ FunctionPassManager FPM;
+ FPM.addPass(WrongFunctionPass());
+
+ auto *F = M->getFunction("foo");
+ EXPECT_DEATH(FPM.run(*F, FAM), "Function @foo changed by WrongFunctionPass without invalidating analyses");
+}
+
+#endif
+
}
Index: llvm/lib/Passes/StandardInstrumentations.cpp
===================================================================
--- llvm/lib/Passes/StandardInstrumentations.cpp
+++ llvm/lib/Passes/StandardInstrumentations.cpp
@@ -24,6 +24,7 @@
#include "llvm/IR/PassInstrumentation.h"
#include "llvm/IR/PassManager.h"
#include "llvm/IR/PrintPasses.h"
+#include "llvm/IR/StructuralHash.h"
#include "llvm/IR/Verifier.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/CrashRecoveryContext.h"
@@ -1048,6 +1049,23 @@
AnalysisKey PreservedCFGCheckerAnalysis::Key;
+struct PreservedFunctionHashAnalysis
+ : public AnalysisInfoMixin<PreservedFunctionHashAnalysis> {
+ static AnalysisKey Key;
+
+ struct FunctionHash {
+ uint64_t Hash;
+ };
+
+ using Result = FunctionHash;
+
+ Result run(Function &F, FunctionAnalysisManager &FAM) {
+ return Result{StructuralHash(F)};
+ }
+};
+
+AnalysisKey PreservedFunctionHashAnalysis::Key;
+
bool PreservedCFGCheckerInstrumentation::CFG::invalidate(
Function &F, const PreservedAnalyses &PA,
FunctionAnalysisManager::Invalidator &) {
@@ -1062,6 +1080,7 @@
return;
FAM.registerPass([&] { return PreservedCFGCheckerAnalysis(); });
+ FAM.registerPass([&] { return PreservedFunctionHashAnalysis(); });
PIC.registerBeforeNonSkippedPassCallback(
[this, &FAM](StringRef P, Any IR) {
@@ -1075,6 +1094,8 @@
// Make sure a fresh CFG snapshot is available before the pass.
FAM.getResult<PreservedCFGCheckerAnalysis>(*const_cast<Function *>(*F));
+ FAM.getResult<PreservedFunctionHashAnalysis>(
+ *const_cast<Function *>(*F));
});
PIC.registerAfterPassInvalidatedCallback(
@@ -1094,12 +1115,27 @@
#endif
(void)this;
- const auto **F = any_cast<const Function *>(&IR);
- if (!F)
+ const auto **MaybeF = any_cast<const Function *>(&IR);
+ if (!MaybeF)
+ return;
+ Function &F = *const_cast<Function *>(*MaybeF);
+
+ // FIXME: we shouldn't have to check PassPA, we should just be able to check
+ // if the cached analysis exists, but currently PassInstrumentation runs
+ // before analyses are invalidated.
+ if (!PassPA.allAnalysesInSetPreserved<AllAnalysesOn<Function>>())
return;
- if (!PassPA.allAnalysesInSetPreserved<CFGAnalyses>() &&
- !PassPA.allAnalysesInSetPreserved<AllAnalysesOn<Function>>())
+ if (auto *HashBefore =
+ FAM.getCachedResult<PreservedFunctionHashAnalysis>(F)) {
+ if (HashBefore->Hash != StructuralHash(F)) {
+ report_fatal_error(formatv(
+ "Function @{0} changed by {1} without invalidating analyses",
+ F.getName(), P));
+ }
+ }
+
+ if (!PassPA.allAnalysesInSetPreserved<CFGAnalyses>())
return;
auto CheckCFG = [](StringRef Pass, StringRef FuncName,
@@ -1115,10 +1151,9 @@
report_fatal_error(Twine("CFG unexpectedly changed by ", Pass));
};
- if (auto *GraphBefore = FAM.getCachedResult<PreservedCFGCheckerAnalysis>(
- *const_cast<Function *>(*F)))
- CheckCFG(P, (*F)->getName(), *GraphBefore,
- CFG(*F, /* TrackBBLifetime */ false));
+ if (auto *GraphBefore = FAM.getCachedResult<PreservedCFGCheckerAnalysis>(F))
+ CheckCFG(P, F.getName(), *GraphBefore,
+ CFG(&F, /* TrackBBLifetime */ false));
});
}
@@ -2154,18 +2189,17 @@
TimePasses.registerCallbacks(PIC);
OptNone.registerCallbacks(PIC);
OptPassGate.registerCallbacks(PIC);
- if (FAM)
- PreservedCFGChecker.registerCallbacks(PIC, *FAM);
PrintChangedIR.registerCallbacks(PIC);
PseudoProbeVerification.registerCallbacks(PIC);
if (VerifyEach)
Verify.registerCallbacks(PIC);
PrintChangedDiff.registerCallbacks(PIC);
WebsiteChangeReporter.registerCallbacks(PIC);
-
ChangeTester.registerCallbacks(PIC);
-
PrintCrashIR.registerCallbacks(PIC);
+ if (FAM)
+ PreservedCFGChecker.registerCallbacks(PIC, *FAM);
+
// TimeProfiling records the pass running time cost.
// Its 'BeforePassCallback' can be appended at the tail of all the
// BeforeCallbacks by calling `registerCallbacks` in the end.
Index: llvm/lib/IR/StructuralHash.cpp
===================================================================
--- llvm/lib/IR/StructuralHash.cpp
+++ llvm/lib/IR/StructuralHash.cpp
@@ -1,13 +1,10 @@
-//===-- StructuralHash.cpp - IR Hash for expensive checks -------*- C++ -*-===//
+//===-- StructuralHash.cpp - IR Hashing -------------------------*- 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
//
//===----------------------------------------------------------------------===//
-//
-
-#ifdef EXPENSIVE_CHECKS
#include "llvm/IR/StructuralHash.h"
#include "llvm/IR/Function.h"
@@ -77,5 +74,3 @@
H.update(M);
return H.getHash();
}
-
-#endif
Index: llvm/include/llvm/IR/StructuralHash.h
===================================================================
--- llvm/include/llvm/IR/StructuralHash.h
+++ llvm/include/llvm/IR/StructuralHash.h
@@ -1,4 +1,4 @@
-//===- llvm/IR/StructuralHash.h - IR Hash for expensive checks --*- C++ -*-===//
+//===- llvm/IR/StructuralHash.h - IR Hashing --------------------*- C++ -*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
@@ -14,11 +14,8 @@
#ifndef LLVM_IR_STRUCTURALHASH_H
#define LLVM_IR_STRUCTURALHASH_H
-#ifdef EXPENSIVE_CHECKS
-
#include <cstdint>
-// This header is only meant to be used when -DEXPENSIVE_CHECKS is set
namespace llvm {
class Function;
@@ -30,5 +27,3 @@
} // end namespace llvm
#endif
-
-#endif // LLVM_IR_STRUCTURALHASH_H
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
