From a12b4db5659c232c5a7149e59d1fe80ad265c55f Mon Sep 17 00:00:00 2001 From: serge-sans-paille Date: Thu, 27 Aug 2020 17:31:54 +0200 Subject: [PATCH] (Expensive) Check for Loop, SCC and Region pass return status This generalizes the logic introduced in https://reviews.llvm.org/D80916 to other passes. It's needed by https://reviews.llvm.org/D86442 to assert passes correctly report their status. Differential Revision: https://reviews.llvm.org/D86589 --- include/llvm/IR/StructuralHash.h | 34 +++++++++++++ lib/Analysis/CallGraphSCCPass.cpp | 22 ++++++-- lib/Analysis/LoopPass.cpp | 13 +++++ lib/Analysis/RegionPass.cpp | 19 ++++++- lib/IR/CMakeLists.txt | 1 + lib/IR/LegacyPassManager.cpp | 72 +------------------------- lib/IR/StructuralHash.cpp | 84 +++++++++++++++++++++++++++++++ 7 files changed, 169 insertions(+), 76 deletions(-) create mode 100644 include/llvm/IR/StructuralHash.h create mode 100644 lib/IR/StructuralHash.cpp diff --git a/include/llvm/IR/StructuralHash.h b/include/llvm/IR/StructuralHash.h new file mode 100644 index 00000000000..eb63a214031 --- /dev/null +++ b/include/llvm/IR/StructuralHash.h @@ -0,0 +1,34 @@ +//===- llvm/IR/StructuralHash.h - IR Hash for expensive checks --*- 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 +// +//===----------------------------------------------------------------------===// +// +// This file provides hashing of the LLVM IR structure to be used to check +// Passes modification status. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_IR_STRUCTURALHASH_H +#define LLVM_IR_STRUCTURALHASH_H + +#ifdef EXPENSIVE_CHECKS + +#include + +// This header is only meant to be used when -DEXPENSIVE_CHECKS is set +namespace llvm { + +class Function; +class Module; + +uint64_t StructuralHash(const Function &F); +uint64_t StructuralHash(const Module &M); + +} // end namespace llvm + +#endif + +#endif // LLVM_IR_STRUCTURALHASH_H diff --git a/lib/Analysis/CallGraphSCCPass.cpp b/lib/Analysis/CallGraphSCCPass.cpp index 91f8029cc32..17dd4dd389d 100644 --- a/lib/Analysis/CallGraphSCCPass.cpp +++ b/lib/Analysis/CallGraphSCCPass.cpp @@ -28,6 +28,7 @@ #include "llvm/IR/Module.h" #include "llvm/IR/OptBisect.h" #include "llvm/IR/PassTimingInfo.h" +#include "llvm/IR/StructuralHash.h" #include "llvm/Pass.h" #include "llvm/Support/CommandLine.h" #include "llvm/Support/Debug.h" @@ -466,11 +467,24 @@ bool CGPassManager::RunAllPassesOnSCC(CallGraphSCC &CurSCC, CallGraph &CG, initializeAnalysisImpl(P); - // Actually run this pass on the current SCC. - Changed |= RunPassOnSCC(P, CurSCC, CG, - CallGraphUpToDate, DevirtualizedCall); +#ifdef EXPENSIVE_CHECKS + uint64_t RefHash = StructuralHash(CG.getModule()); +#endif - if (Changed) + // Actually run this pass on the current SCC. + bool LocalChanged = + RunPassOnSCC(P, CurSCC, CG, CallGraphUpToDate, DevirtualizedCall); + + Changed |= LocalChanged; + +#ifdef EXPENSIVE_CHECKS + if (!LocalChanged && (RefHash != StructuralHash(CG.getModule()))) { + llvm::errs() << "Pass modifies its input and doesn't report it: " + << P->getPassName() << "\n"; + llvm_unreachable("Pass modifies its input and doesn't report it"); + } +#endif + if (LocalChanged) dumpPassInfo(P, MODIFICATION_MSG, ON_CG_MSG, ""); dumpPreservedSet(P); diff --git a/lib/Analysis/LoopPass.cpp b/lib/Analysis/LoopPass.cpp index 520f06003dd..317b9577d79 100644 --- a/lib/Analysis/LoopPass.cpp +++ b/lib/Analysis/LoopPass.cpp @@ -20,6 +20,7 @@ #include "llvm/IR/OptBisect.h" #include "llvm/IR/PassManager.h" #include "llvm/IR/PassTimingInfo.h" +#include "llvm/IR/StructuralHash.h" #include "llvm/InitializePasses.h" #include "llvm/Support/Debug.h" #include "llvm/Support/TimeProfiler.h" @@ -191,7 +192,19 @@ bool LPPassManager::runOnFunction(Function &F) { { PassManagerPrettyStackEntry X(P, *CurrentLoop->getHeader()); TimeRegion PassTimer(getPassTimer(P)); +#ifdef EXPENSIVE_CHECKS + uint64_t RefHash = StructuralHash(F); +#endif LocalChanged = P->runOnLoop(CurrentLoop, *this); + +#ifdef EXPENSIVE_CHECKS + if (!LocalChanged && (RefHash != StructuralHash(F))) { + llvm::errs() << "Pass modifies its input and doesn't report it: " + << P->getPassName() << "\n"; + llvm_unreachable("Pass modifies its input and doesn't report it"); + } +#endif + Changed |= LocalChanged; if (EmitICRemark) { unsigned NewSize = F.getInstructionCount(); diff --git a/lib/Analysis/RegionPass.cpp b/lib/Analysis/RegionPass.cpp index 6c0d17b45c6..1e1971f119a 100644 --- a/lib/Analysis/RegionPass.cpp +++ b/lib/Analysis/RegionPass.cpp @@ -15,6 +15,7 @@ #include "llvm/Analysis/RegionPass.h" #include "llvm/IR/OptBisect.h" #include "llvm/IR/PassTimingInfo.h" +#include "llvm/IR/StructuralHash.h" #include "llvm/Support/Debug.h" #include "llvm/Support/Timer.h" #include "llvm/Support/raw_ostream.h" @@ -90,15 +91,29 @@ bool RGPassManager::runOnFunction(Function &F) { initializeAnalysisImpl(P); + bool LocalChanged = false; { PassManagerPrettyStackEntry X(P, *CurrentRegion->getEntry()); TimeRegion PassTimer(getPassTimer(P)); - Changed |= P->runOnRegion(CurrentRegion, *this); +#ifdef EXPENSIVE_CHECKS + uint64_t RefHash = StructuralHash(F); +#endif + LocalChanged = P->runOnRegion(CurrentRegion, *this); + +#ifdef EXPENSIVE_CHECKS + if (!LocalChanged && (RefHash != StructuralHash(F))) { + llvm::errs() << "Pass modifies its input and doesn't report it: " + << P->getPassName() << "\n"; + llvm_unreachable("Pass modifies its input and doesn't report it"); + } +#endif + + Changed |= LocalChanged; } if (isPassDebuggingExecutionsOrMore()) { - if (Changed) + if (LocalChanged) dumpPassInfo(P, MODIFICATION_MSG, ON_REGION_MSG, skipThisRegion ? "" : CurrentRegion->getNameStr()); diff --git a/lib/IR/CMakeLists.txt b/lib/IR/CMakeLists.txt index 8fcc10fa38a..49805d5b8c2 100644 --- a/lib/IR/CMakeLists.txt +++ b/lib/IR/CMakeLists.txt @@ -47,6 +47,7 @@ add_llvm_component_library(LLVMCore SafepointIRVerifier.cpp ProfileSummary.cpp Statepoint.cpp + StructuralHash.cpp Type.cpp TypeFinder.cpp Use.cpp diff --git a/lib/IR/LegacyPassManager.cpp b/lib/IR/LegacyPassManager.cpp index 96434ae3306..8d9ed917bb6 100644 --- a/lib/IR/LegacyPassManager.cpp +++ b/lib/IR/LegacyPassManager.cpp @@ -20,6 +20,7 @@ #include "llvm/IR/LegacyPassNameParser.h" #include "llvm/IR/Module.h" #include "llvm/IR/PassTimingInfo.h" +#include "llvm/IR/StructuralHash.h" #include "llvm/Support/Chrono.h" #include "llvm/Support/CommandLine.h" #include "llvm/Support/Debug.h" @@ -1475,75 +1476,6 @@ void FPPassManager::dumpPassStructure(unsigned Offset) { } } -#ifdef EXPENSIVE_CHECKS -namespace { -namespace details { - -// Basic hashing mechanism to detect structural change to the IR, used to verify -// pass return status consistency with actual change. Loosely copied from -// llvm/lib/Transforms/Utils/FunctionComparator.cpp - -class StructuralHash { - uint64_t Hash = 0x6acaa36bef8325c5ULL; - - void update(uint64_t V) { Hash = hashing::detail::hash_16_bytes(Hash, V); } - -public: - StructuralHash() = default; - - void update(Function &F) { - if (F.empty()) - return; - - update(F.isVarArg()); - update(F.arg_size()); - - SmallVector BBs; - SmallPtrSet VisitedBBs; - - BBs.push_back(&F.getEntryBlock()); - VisitedBBs.insert(BBs[0]); - while (!BBs.empty()) { - const BasicBlock *BB = BBs.pop_back_val(); - update(45798); // Block header - for (auto &Inst : *BB) - update(Inst.getOpcode()); - - const Instruction *Term = BB->getTerminator(); - for (unsigned i = 0, e = Term->getNumSuccessors(); i != e; ++i) { - if (!VisitedBBs.insert(Term->getSuccessor(i)).second) - continue; - BBs.push_back(Term->getSuccessor(i)); - } - } - } - - void update(Module &M) { - for (Function &F : M) - update(F); - } - - uint64_t getHash() const { return Hash; } -}; - -} // namespace details - -uint64_t StructuralHash(Function &F) { - details::StructuralHash H; - H.update(F); - return H.getHash(); -} - -uint64_t StructuralHash(Module &M) { - details::StructuralHash H; - H.update(M); - return H.getHash(); -} - -} // end anonymous namespace - -#endif - /// Execute all of the passes scheduled for execution by invoking /// runOnFunction method. Keep track of whether any of the passes modifies /// the function, and if so, return true. @@ -1590,7 +1522,7 @@ bool FPPassManager::runOnFunction(Function &F) { if (!LocalChanged && (RefHash != StructuralHash(F))) { llvm::errs() << "Pass modifies its input and doesn't report it: " << FP->getPassName() << "\n"; - assert(false && "Pass modifies its input and doesn't report it."); + llvm_unreachable("Pass modifies its input and doesn't report it"); } #endif diff --git a/lib/IR/StructuralHash.cpp b/lib/IR/StructuralHash.cpp new file mode 100644 index 00000000000..5a6e0745132 --- /dev/null +++ b/lib/IR/StructuralHash.cpp @@ -0,0 +1,84 @@ +//===-- StructuralHash.cpp - IR Hash for expensive checks -------*- 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" +#include "llvm/IR/Module.h" + +using namespace llvm; + +namespace { +namespace details { + +// Basic hashing mechanism to detect structural change to the IR, used to verify +// pass return status consistency with actual change. Loosely copied from +// llvm/lib/Transforms/Utils/FunctionComparator.cpp + +class StructuralHash { + uint64_t Hash = 0x6acaa36bef8325c5ULL; + + void update(uint64_t V) { Hash = hashing::detail::hash_16_bytes(Hash, V); } + +public: + StructuralHash() = default; + + void update(const Function &F) { + if (F.empty()) + return; + + update(F.isVarArg()); + update(F.arg_size()); + + SmallVector BBs; + SmallPtrSet VisitedBBs; + + BBs.push_back(&F.getEntryBlock()); + VisitedBBs.insert(BBs[0]); + while (!BBs.empty()) { + const BasicBlock *BB = BBs.pop_back_val(); + update(45798); // Block header + for (auto &Inst : *BB) + update(Inst.getOpcode()); + + const Instruction *Term = BB->getTerminator(); + for (unsigned i = 0, e = Term->getNumSuccessors(); i != e; ++i) { + if (!VisitedBBs.insert(Term->getSuccessor(i)).second) + continue; + BBs.push_back(Term->getSuccessor(i)); + } + } + } + + void update(const Module &M) { + for (const Function &F : M) + update(F); + } + + uint64_t getHash() const { return Hash; } +}; + +} // namespace details + +} // namespace + +uint64_t llvm::StructuralHash(const Function &F) { + details::StructuralHash H; + H.update(F); + return H.getHash(); +} + +uint64_t llvm::StructuralHash(const Module &M) { + details::StructuralHash H; + H.update(M); + return H.getHash(); +} + +#endif