[llvm] 20a7ea4 - [StandardInstrumentations] Verify function doesn't change if analyses are preserved
Arthur Eubanks via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 15 13:17:23 PDT 2023
Author: Arthur Eubanks
Date: 2023-03-15T13:13:08-07:00
New Revision: 20a7ea49f40e2d47eb6b87acf835782dece92f08
URL: https://github.com/llvm/llvm-project/commit/20a7ea49f40e2d47eb6b87acf835782dece92f08
DIFF: https://github.com/llvm/llvm-project/commit/20a7ea49f40e2d47eb6b87acf835782dece92f08.diff
LOG: [StandardInstrumentations] Verify function doesn't change if analyses are preserved
Reuse StructuralHash and allow it to be used in non-expensive checks builds.
Move PreservedAnalysisChecker further down StandardInstrumentations so other Instrumentations (e.g. printing) have a chance to run before PreservedAnalysisChecker crashes.
Reviewed By: nikic
Differential Revision: https://reviews.llvm.org/D146003
Added:
Modified:
llvm/include/llvm/IR/StructuralHash.h
llvm/lib/IR/StructuralHash.cpp
llvm/lib/Passes/StandardInstrumentations.cpp
llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-markloopasdeleted.ll
llvm/unittests/IR/PassManagerTest.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/IR/StructuralHash.h b/llvm/include/llvm/IR/StructuralHash.h
index eb63a21403100..1bdeb85afa3c5 100644
--- a/llvm/include/llvm/IR/StructuralHash.h
+++ b/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 @@ uint64_t StructuralHash(const Module &M);
} // end namespace llvm
#endif
-
-#endif // LLVM_IR_STRUCTURALHASH_H
diff --git a/llvm/lib/IR/StructuralHash.cpp b/llvm/lib/IR/StructuralHash.cpp
index 0a3c95bdc0884..077f8b6562f5e 100644
--- a/llvm/lib/IR/StructuralHash.cpp
+++ b/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 @@ uint64_t llvm::StructuralHash(const Module &M) {
H.update(M);
return H.getHash();
}
-
-#endif
diff --git a/llvm/lib/Passes/StandardInstrumentations.cpp b/llvm/lib/Passes/StandardInstrumentations.cpp
index b2df7961bc9be..8008986046188 100644
--- a/llvm/lib/Passes/StandardInstrumentations.cpp
+++ b/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"
@@ -1049,6 +1050,23 @@ struct PreservedCFGCheckerAnalysis
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 &) {
@@ -1063,6 +1081,7 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
return;
FAM.registerPass([&] { return PreservedCFGCheckerAnalysis(); });
+ FAM.registerPass([&] { return PreservedFunctionHashAnalysis(); });
PIC.registerBeforeNonSkippedPassCallback(
[this, &FAM](StringRef P, Any IR) {
@@ -1076,6 +1095,8 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
// 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(
@@ -1095,9 +1116,19 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
#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);
+
+ 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));
+ }
+ }
auto CheckCFG = [](StringRef Pass, StringRef FuncName,
const CFG &GraphBefore, const CFG &GraphAfter) {
@@ -1112,10 +1143,9 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
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));
});
}
@@ -2151,18 +2181,17 @@ void StandardInstrumentations::registerCallbacks(
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.
diff --git a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-markloopasdeleted.ll b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-markloopasdeleted.ll
index d97ef782daebf..c8e1291b9cd55 100644
--- a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-markloopasdeleted.ll
+++ b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-markloopasdeleted.ll
@@ -1,4 +1,4 @@
-; RUN: opt < %s -enable-loop-distribute -passes='loop-distribute,loop-mssa(simple-loop-unswitch<nontrivial>),loop-distribute' -o /dev/null -S -verify-analysis-invalidation -debug-pass-manager=verbose 2>&1 | FileCheck %s
+; RUN: opt < %s -enable-loop-distribute -passes='loop-distribute,loop-mssa(simple-loop-unswitch<nontrivial>),loop-distribute' -o /dev/null -S -verify-analysis-invalidation=0 -debug-pass-manager=verbose 2>&1 | FileCheck %s
; Running loop-distribute will result in LoopAccessAnalysis being required and
@@ -29,7 +29,6 @@
;
; CHECK: Invalidating analysis: LoopAccessAnalysis on test6
; CHECK-NEXT: Running pass: LoopDistributePass on test6
-; CHECK-NEXT: Running analysis: PreservedCFGCheckerAnalysis on test6
; CHECK-NEXT: Running analysis: LoopAccessAnalysis on test6
diff --git a/llvm/unittests/IR/PassManagerTest.cpp b/llvm/unittests/IR/PassManagerTest.cpp
index bae5d46ecd11c..4c8be3702bf0f 100644
--- a/llvm/unittests/IR/PassManagerTest.cpp
+++ b/llvm/unittests/IR/PassManagerTest.cpp
@@ -950,4 +950,37 @@ TEST_F(PassManagerTest, FunctionPassCFGCheckerWrapped) {
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
+
}
More information about the llvm-commits
mailing list