[NewPM] Disable PreserveCFGChecker and add regression unit tests

[yrouban]

Summary

The design of the PreserveCFG Checker (landed as D81558 [NewPM] Introduce PreserveCFG check) has a fundamental flaw which makes it incorrect. The checker is based on the PreservedAnalyses result returned by functional passes: if CFGAnalyses is in the returned PreservedAnalyses set, then the checker asserts that the CFG snapshot saved before the pass is equal to the CFG snapshot taken after the the pass. The problem is in passes that change CFG and invalidate CFGAnalyses on their own. Such passes do not return CFGanalyses in the returned PreservedAnalyses. So the checker mistakenly expects CFG unchanged. As an example see the class TestSimplifyCFGInvalidatingAnalysisPass in the new tests.

It is interesting that the bug was not found in LLVM. That is because the CFG checker ran only if CFGAnalyses was checked incorrectly:

if (!PassPA.allAnalysesInSetPreserved<CFGAnalyses>())
  return;

but must be checked as follows:

auto PAC = PA.getChecker<PreservedCFGCheckerAnalysis>();
if (!(PAC.preserved() || PAC.preservedSet<AllAnalysesOn<Function>>() || PAC.preservedSet<CFGAnalyses>())
  return;

This fix and a fully redesigned checker will be sent as a separate follow-up patch.

[skatkov]

Do I understand correctly that with -verify-cfg-preserved=true all new tests fail?

I would suggest to move new tests to separate test file. Mark it as expected to fail and do not change the value of -verify-cfg-preserved in this patch.
Then in a separate commit, turn the option be default and remove expected fail.
After that fix the issue and turn the option to on back.

If it is possible actually :)

llvm/unittests/IR/PassManagerTest.cpp

811

could you please be more precise. Specifically state explicitly what is "accordingly".

[yrouban]

In D91324#2398657, @skatkov wrote:

Do I understand correctly that with -verify-cfg-preserved=true all new tests fail?

Only PassManagerTest.FunctionPassCFGCheckerInvalidateAnalysis does fail.
The other two (PassManagerTest.FunctionPassCFGCheckerWrapped and PassManagerTest.FunctionPassCFGChecker) pass.

I would suggest to move new tests to separate test file. Mark it as expected to fail and do not change the value of -verify-cfg-preserved in this patch.
Then in a separate commit, turn the option be default and remove expected fail.
After that fix the issue and turn the option to on back.

If it is possible actually :)

I did not find a way to exclude except changing the line 'TEST_F(PassManagerTest, ...) to just a func definition' to skip this test. But this way the tests will not be run.
So, if you do not mind, I will land as is.

[skatkov]

I do not mind.

[kuhar]

LGTM

[awarzynski]

Hi @yrouban, thank you for working on this! Unfortunately this breaks shared library builds (i.e. with BUILD_SHARED_LIBS set to ON). This is causing one of our buildbots to fail:

• http://lab.llvm.org:8011/#/builders/66/builds/555

Admittedly, it's been failing in the past 24hrs for other reasons too :)

As the fix seemed trivial, I took the liberty of submitting a fixing patch without a review: https://github.com/llvm/llvm-project/commit/ccf500ce00c5768ae98248a8de90408488235aa7. Tested with BUILD_SHARED_LIBS and it builds fine. But I'm not that familiar with this codebase, so please comment/revert/update if I missed sth. Ta!

[yrouban]

In D91324#2402069, @awarzynski wrote:

Hi @yrouban, thank you for working on this! Unfortunately this breaks shared library builds (i.e. with BUILD_SHARED_LIBS set to ON). This is causing one of our buildbots to fail:

• http://lab.llvm.org:8011/#/builders/66/builds/555

Admittedly, it's been failing in the past 24hrs for other reasons too :)

As the fix seemed trivial, I took the liberty of submitting a fixing patch without a review: https://github.com/llvm/llvm-project/commit/ccf500ce00c5768ae98248a8de90408488235aa7. Tested with BUILD_SHARED_LIBS and it builds fine. But I'm not that familiar with this codebase, so please comment/revert/update if I missed sth. Ta!

Thanks. I believe you did all right. There were several unresolved symbols from SimplifyCFG reported as needed by PassManagerTest.cpp.

llvm/lib/Passes/StandardInstrumentations.cpp

EnableOptnone("enable-npm-optnone", cl::init(true),
                   cl::desc("Enable skipping optional passes optnone functions "
                            "under new pass manager"));
 
 cl::opt<bool> PreservedCFGCheckerInstrumentation::VerifyPreservedCFG(
     "verify-cfg-preserved", cl::Hidden,
 #ifdef NDEBUG
     cl::init(false));
 #else
-    cl::init(true));
+    cl::init(false));
 #endif
 
 // FIXME: Change `-debug-pass-manager` from boolean to enum type. Similar to
 // `-debug-pass` in legacy PM.
 static cl::opt<bool>
     DebugPMVerbose("debug-pass-manager-verbose", cl::Hidden, cl::init(false),
                    cl::desc("Print all pass management debugging information. "
                             "`-debug-pass-manager` must also be specified"));

llvm/unittests/IR/PassManagerTest.cpp

 //===- llvm/unittest/IR/PassManager.cpp - PassManager tests ---------------===//
 //
 // 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/IR/PassManager.h"
+#include "llvm/Analysis/AssumptionCache.h"
+#include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/AsmParser/Parser.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassManagerImpl.h"
+#include "llvm/Passes/StandardInstrumentations.h"
 #include "llvm/Support/SourceMgr.h"
+#include "llvm/Transforms/Scalar/SimplifyCFG.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 
 class TestFunctionAnalysis : public AnalysisInfoMixin<TestFunctionAnalysis> {
 public:
TEST_F(PassManagerTest, IndirectAnalysisInvalidation) {
   // There are five instructions in the module and we add the count four
   // times.
   EXPECT_EQ(5 * 4, InstrCount);
 
   // There are three functions and we count them four times for each of the
   // three functions.
   EXPECT_EQ(3 * 4 * 3, FunctionCount);
 }
+
+// Run SimplifyCFGPass that makes CFG changes and reports PreservedAnalyses
+// accordingly. So the CFGChecker does not complain.
+TEST_F(PassManagerTest, FunctionPassCFGChecker) {
+  LLVMContext Context;
+  // SimplifyCFG changes this function to
+  // define void @foo {next: ret void}
+  auto M = parseIR(Context, "define void @foo() {\n"
+                            "  br label %next\n"
+                            "next:\n"
+                            "  br label %exit\n"
+                            "exit:\n"
+                            "  ret void\n"
+                            "}\n");
+
+  auto *F = M->getFunction("foo");
+  FunctionAnalysisManager FAM(/*DebugLogging*/ true);
+  FunctionPassManager FPM(/*DebugLogging*/ true);
+  PassInstrumentationCallbacks PIC;
+  StandardInstrumentations SI(/*DebugLogging*/ true);
+  SI.registerCallbacks(PIC);
+  FAM.registerPass([&] { return PassInstrumentationAnalysis(&PIC); });
+  FAM.registerPass([&] { return AssumptionAnalysis(); });
+  FAM.registerPass([&] { return TargetIRAnalysis(); });
+
+  FPM.addPass(SimplifyCFGPass());
+  FPM.run(*F, FAM);
+}
+
+// FunctionPass that manually invalidates analyses and always returns
+// PreservedAnalyses::all().
+struct TestSimplifyCFGInvalidatingAnalysisPass
+    : PassInfoMixin<TestSimplifyCFGInvalidatingAnalysisPass> {
+  PreservedAnalyses run(Function &F, FunctionAnalysisManager &FAM) {
+    // Run SimplifyCFG and if it changes CFG then invalidate the CFG analysis.
+    // This allows to return PreserveAnalysis::all().
+    PreservedAnalyses PA = CFGSimplifier.run(F, FAM);
+    FAM.invalidate(F, PA);
+    return PreservedAnalyses::all();
+  }
+
+  SimplifyCFGPass CFGSimplifier;
+};
+
+// Run TestSimplifyCFGInvalidatingAnalysisPass which changes CFG by running
+// SimplifyCFGPass then manually invalidates analyses and always returns
+// PreservedAnalyses::all(). CFGChecker does not complain because it resets
+// its saved CFG snapshot when the analyses are invalidated manually.
+TEST_F(PassManagerTest, FunctionPassCFGCheckerInvalidateAnalysis) {
+  LLVMContext Context;
+  // SimplifyCFG changes this function to
+  // define void @foo {next: ret void}
+  auto M = parseIR(Context, "define void @foo() {\n"
+                            "  br label %next\n"
+                            "next:\n"
+                            "  br label %exit\n"
+                            "exit:\n"
+                            "  ret void\n"
+                            "}\n");
+
+  auto *F = M->getFunction("foo");
+  FunctionAnalysisManager FAM(/*DebugLogging*/ true);
+  FunctionPassManager FPM(/*DebugLogging*/ true);
+  PassInstrumentationCallbacks PIC;
+  StandardInstrumentations SI(/*DebugLogging*/ true);
+  SI.registerCallbacks(PIC);
+  FAM.registerPass([&] { return PassInstrumentationAnalysis(&PIC); });
+  FAM.registerPass([&] { return AssumptionAnalysis(); });
+  FAM.registerPass([&] { return TargetIRAnalysis(); });
+
+  FPM.addPass(TestSimplifyCFGInvalidatingAnalysisPass());
+  FPM.run(*F, FAM);
+}
+
+// Wrap a FunctionPassManager running SimplifyCFG pass with another
+// FunctionPassManager.
+struct TestSimplifyCFGWrapperPass : PassInfoMixin<TestSimplifyCFGWrapperPass> {
+  TestSimplifyCFGWrapperPass(FunctionPassManager &InnerPM) : InnerPM(InnerPM) {}
+
+  PreservedAnalyses run(Function &F, FunctionAnalysisManager &FAM) {
+    // Here we simulate exactly what FunctionPassManager::run() does but
+    // instead of running all passes from InnerPM.Passes we run them in bulk
+    // by calling InnerPM.run().
+    PreservedAnalyses PA = PreservedAnalyses::all();
+    PassInstrumentation PI = FAM.getResult<PassInstrumentationAnalysis>(F);
+
+    if (!PI.runBeforePass<Function>(InnerPM, F))
+      return PreservedAnalyses::all();
+
+    PreservedAnalyses PassPA = InnerPM.run(F, FAM);
+    PI.runAfterPass(InnerPM, F, PassPA);
+    FAM.invalidate(F, PassPA);
+    PA.intersect(PassPA);
+    PA.preserveSet<AllAnalysesOn<Function>>();
+    return PA;
+  }
+
+  FunctionPassManager &InnerPM;
+};
+
+// Run TestSimplifyCFGWrapperPass which simulates behavior of
+// FunctionPassManager::run() except that it runs all passes at once by calling
+// an inner pass manager's passes with PassManager::run(). This is how one pass
+// manager is expected to wrap another pass manager.
+// SimplifyCFGPass, which is called by the inner pass manager, changes the CFG.
+// The CFGChecker's AfterPassCallback, run right after SimplifyCFGPass, does not
+// complain because CFGAnalyses is not in the PreservedAnalises set returned by
+// SimplifyCFGPass. Then the CFG analysis is invalidated by the analysis manager
+// according to the PreservedAnalises set. Further calls to CFGChecker's
+// AfterPassCallback see that all analyses for the current function are
+// preserved but there is no CFG snapshot available (i.e.
+// AM.getCachedResult<PreservedCFGCheckerAnalysis>(F) returns nullptr).
+TEST_F(PassManagerTest, FunctionPassCFGCheckerWrapped) {
+  LLVMContext Context;
+  // SimplifyCFG changes this function to
+  // define void @foo {next: ret void}
+  auto M = parseIR(Context, "define void @foo() {\n"
+                            "  br label %next\n"
+                            "next:\n"
+                            "  br label %exit\n"
+                            "exit:\n"
+                            "  ret void\n"
+                            "}\n");
+
+  auto *F = M->getFunction("foo");
+  FunctionAnalysisManager FAM(/*DebugLogging*/ true);
+  FunctionPassManager FPM(/*DebugLogging*/ true);
+  PassInstrumentationCallbacks PIC;
+  StandardInstrumentations SI(/*DebugLogging*/ true);
+  SI.registerCallbacks(PIC);
+  FAM.registerPass([&] { return PassInstrumentationAnalysis(&PIC); });
+  FAM.registerPass([&] { return AssumptionAnalysis(); });
+  FAM.registerPass([&] { return TargetIRAnalysis(); });
+
+  FunctionPassManager InnerFPM(/*DebugLogging*/ true);
+  InnerFPM.addPass(SimplifyCFGPass());
+
+  FPM.addPass(TestSimplifyCFGWrapperPass(InnerFPM));
+  FPM.run(*F, FAM);
+}
 }