[NewPM] Only verify loop for nonskipped user loop pass

[ychen]

Summary

No verification for pass mangers since it is not needed.
No verification for skipped loop pass since the asserted condition is not used.

Ideally, we should use add a BeforeNonSkippedPass callback for this. But
the callback API need to modification to pass required verification
argument. Let's defer this further until more similar needs arise so a
commonly useful API could be decided. One have in mind is MachineVerifier
which needs machine analysis manager to work.

[ychen]

llvm/test/Feature/optnone-opt.ll

37

This is to activate LCSSA analysis.

[asbirlea]

lg.

[aeubanks]

It seems like the verification is to check either that on the first round, LoopCanonicalizationFPM worked, or on iterations after the first, the loop pass maintained whatever loop invariants.
Since the LoopPassManager itself doesn't check for L->isRecursivelyLCSSAForm(LAR.DT, LI), I don't think we should guard the check with !isSpecialPass(Pass.name(), {"PassManager"}).

Maybe it makes more sense to mark LoopSimplifyPass and LCSSAPass as required? Since we don't know ahead of time if any loop passes in the pass manager are required (well we could, but that's a different issue).

llvm/include/llvm/Transforms/Scalar/LoopPassManager.h

326–330

move this up even more? right after Loop *L = Worklist.pop_back_val();

[ebrevnov]

In D84977#2186550, @aeubanks wrote:

It seems like the verification is to check either that on the first round, LoopCanonicalizationFPM worked, or on iterations after the first, the loop pass maintained whatever loop invariants.
Since the LoopPassManager itself doesn't check for L->isRecursivelyLCSSAForm(LAR.DT, LI), I don't think we should guard the check with !isSpecialPass(Pass.name(), {"PassManager"}).

If I'm not mistaken LoopPassManager does check loop invariant after each pass (lines 63-68 in LoopPassManager.cpp). If it didn't we would need to fix that, right? I think FunctionToLoopPassAdaptor should redirect to LoopPassManager for running loop passes instead of duplicating its functionality. Ideas?

Maybe it makes more sense to mark LoopSimplifyPass and LCSSAPass as required? Since we don't know ahead of time if any loop passes in the pass manager are required (well we could, but that's a different issue).

But that would affect much more places than needed. Instead LoopPassManager could run canonicalization passes before first non-skipped pass. Should be easy to support.

[ebrevnov]

llvm/include/llvm/Transforms/Scalar/LoopPassManager.h

334

Is that tabs?

[ychen]

• Use non-skipped-pass callback. For wrapped loop pass manager, it is called before each non-skipped loop pass instead of once before the loop pass manager begin to run.
• The introduced push/pop thing could be used elsewhere like MachineVerifier. I think it should work well enough for the use cases we have now and no need to change the callback API everywhere. Thoughts?

[ychen]

In D84977#2186550, @aeubanks wrote:

It seems like the verification is to check either that on the first round, LoopCanonicalizationFPM worked, or on iterations after the first, the loop pass maintained whatever loop invariants.
Since the LoopPassManager itself doesn't check for L->isRecursivelyLCSSAForm(LAR.DT, LI), I don't think we should guard the check with !isSpecialPass(Pass.name(), {"PassManager"}).

Indeed. I was thinking in the context of callbacks which does not apply here.

Maybe it makes more sense to mark LoopSimplifyPass and LCSSAPass as required?

We could but it sounds more like a workaround since we don't mean to run these. Applying verification when it is really needed sounds more appealing IMHO.

[ychen]

The push/pop causes some unit tests to fail. I'll investigate.

[ychen]

• Checking Callback pointer before use

[ychen]

In D84977#2186618, @ebrevnov wrote:

In D84977#2186550, @aeubanks wrote:

It seems like the verification is to check either that on the first round, LoopCanonicalizationFPM worked, or on iterations after the first, the loop pass maintained whatever loop invariants.
Since the LoopPassManager itself doesn't check for L->isRecursivelyLCSSAForm(LAR.DT, LI), I don't think we should guard the check with !isSpecialPass(Pass.name(), {"PassManager"}).

If I'm not mistaken LoopPassManager does check loop invariant after each pass (lines 63-68 in LoopPassManager.cpp). If it didn't we would need to fix that, right? I think FunctionToLoopPassAdaptor should redirect to LoopPassManager for running loop passes instead of duplicating its functionality. Ideas?

An adaptor could wrap a loop pass directly without a loop pass manager. The code in FunctionToLoopPassAdaptor is needed to handle that.

Maybe it makes more sense to mark LoopSimplifyPass and LCSSAPass as required? Since we don't know ahead of time if any loop passes in the pass manager are required (well we could, but that's a different issue).

But that would affect much more places than needed. Instead LoopPassManager could run canonicalization passes before first non-skipped pass. Should be easy to support.

ditto.

[aeubanks]

But if there is a required loop pass (that's kinda weird but possible), this assert will still fail because we tried to run LoopSimplifyPass and LCSSAPass but they didn't actually run (e.g. if the function is optnone).

[ychen]

In D84977#2188199, @aeubanks wrote:

But if there is a required loop pass (that's kinda weird but possible), this assert will still fail because we tried to run LoopSimplifyPass and LCSSAPass but they didn't actually run (e.g. if the function is optnone).

Agree. It would fail explicitly if such irregular usage comes up.

• If we suppress it by running verification depending on if LoopSimplifyPass/LCSSAPass has run, a non-skipped loop pass may break implicitly if LCSSA is assumed.
• We could mark LoopSimplifyPass/LCSSAPass as required, but I would refrain to do so until we really need to.
• The current approach does not make assumptions. It is one step forward in the direction of using PassInstrument to debug/logging. As a side effect, it workarounds this assertion failure.

Actually, we could move LoopCanonicalizationFPM inside the BeforeNonSkippedPassCallback to run once. Then it would be always consistent. I didn't do this because PassID is not portable so putting non-debugging code like LoopCanonicalizationFPM there feels risky on paper (It may be ok in practice).

llvm/include/llvm/IR/PassInstrumentation.h

public:
   ///
   /// On attempt to invalidate just return false. There is nothing to become
   /// invalid here.
   template <typename IRUnitT, typename... ExtraArgsT>
   bool invalidate(IRUnitT &, const class llvm::PreservedAnalyses &,
                   ExtraArgsT...) {
     return false;
   }
+
+  template <typename CallableT>
+  void pushBeforeNonSkippedPassCallback(CallableT C) {
+    if (Callbacks)
+      Callbacks->BeforeNonSkippedPassCallbacks.emplace_back(std::move(C));
+  }
+  void popBeforeNonSkippedPassCallback() {
+    if (Callbacks)
+      Callbacks->BeforeNonSkippedPassCallbacks.pop_back();
+  }
 };
 
 bool isSpecialPass(StringRef PassID, const std::vector<StringRef> &Specials);
 
 } // namespace llvm
 
 #endif

llvm/include/llvm/Transforms/Scalar/LoopPassManager.h

 #include "llvm/Analysis/LoopAnalysisManager.h"
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/Analysis/MemorySSA.h"
 #include "llvm/Analysis/ScalarEvolution.h"
 #include "llvm/Analysis/ScalarEvolutionAliasAnalysis.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/IR/Dominators.h"
+#include "llvm/IR/PassInstrumentation.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/Transforms/Utils/LCSSA.h"
 #include "llvm/Transforms/Utils/LoopSimplify.h"
 #include "llvm/Transforms/Utils/LoopUtils.h"
 
 namespace llvm {
 
 // Forward declarations of an update tracking API used in the pass manager.
PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM) {
     // Register the worklist and loop analysis manager so that loop passes can
     // update them when they mutate the loop nest structure.
     LPMUpdater Updater(Worklist, LAM);
 
     // Add the loop nests in the reverse order of LoopInfo. See method
     // declaration.
     appendLoopsToWorklist(LI, Worklist);
 
+#ifndef NDEBUG
+    PI.pushBeforeNonSkippedPassCallback([&LAR, &LI](StringRef PassID, Any IR) {
+      if (isSpecialPass(PassID, {"PassManager"}))
+        return;
+      assert(any_isa<const Loop *>(IR));
+      const Loop *L = any_cast<const Loop *>(IR);
+      assert(L && "Loop should be valid for printing");
+
+      // Verify the loop structure and LCSSA form before visiting the loop.
+      L->verifyLoop();
+      assert(L->isRecursivelyLCSSAForm(LAR.DT, LI) &&
+             "Loops must remain in LCSSA form!");
+    });
+#endif
+
     do {
       Loop *L = Worklist.pop_back_val();
 
       // Reset the update structure for this loop.
       Updater.CurrentL = L;
       Updater.SkipCurrentLoop = false;
 
 #ifndef NDEBUG
       // Save a parent loop pointer for asserts.
       Updater.ParentL = L->getParentLoop();
-
-      // Verify the loop structure and LCSSA form before visiting the loop.
-      L->verifyLoop();
-      assert(L->isRecursivelyLCSSAForm(LAR.DT, LI) &&
-             "Loops must remain in LCSSA form!");
 #endif
       // Check the PassInstrumentation's BeforePass callbacks before running the
       // pass, skip its execution completely if asked to (callback returns
       // false).
       if (!PI.runBeforePass<Loop>(Pass, *L))
         continue;
 
       PreservedAnalyses PassPA;
       {
         TimeTraceScope TimeScope(Pass.name());
         PassPA = Pass.run(*L, LAM, LAR, Updater);
       }
 
       // Do not pass deleted Loop into the instrumentation.
       if (Updater.skipCurrentLoop())
         PI.runAfterPassInvalidated<Loop>(Pass);
       else
         PI.runAfterPass<Loop>(Pass, *L);
 
       // FIXME: We should verify the set of analyses relevant to Loop passes
       // are preserved.
 
       // If the loop hasn't been deleted, we need to handle invalidation here.
       if (!Updater.skipCurrentLoop())
         // We know that the loop pass couldn't have invalidated any other
         // loop's analyses (that's the contract of a loop pass), so directly
         // handle the loop analysis manager's invalidation here.
         LAM.invalidate(*L, PassPA);
 
       // Then intersect the preserved set so that invalidation of module
       // analyses will eventually occur when the module pass completes.
       PA.intersect(std::move(PassPA));
     } while (!Worklist.empty());
 
+#ifndef NDEBUG
+    PI.popBeforeNonSkippedPassCallback();
+#endif
+
     // By definition we preserve the proxy. We also preserve all analyses on
     // Loops. This precludes *any* invalidation of loop analyses by the proxy,
     // but that's OK because we've taken care to invalidate analyses in the
     // loop analysis manager incrementally above.
     PA.preserveSet<AllAnalysesOn<Loop>>();
     PA.preserve<LoopAnalysisManagerFunctionProxy>();
     // We also preserve the set of standard analyses.
     PA.preserve<DominatorTreeAnalysis>();

llvm/lib/Transforms/Scalar/LoopPassManager.cpp

else
       PI.runAfterPass<Loop>(*Pass, L);
 
     // If the loop was deleted, abort the run and return to the outer walk.
     if (U.skipCurrentLoop()) {
       PA.intersect(std::move(PassPA));
       break;
     }
 
-#ifndef NDEBUG
-    // Verify the loop structure and LCSSA form before visiting the loop.
-    L.verifyLoop();
-    assert(L.isRecursivelyLCSSAForm(AR.DT, AR.LI) &&
-           "Loops must remain in LCSSA form!");
-#endif
-
     // Update the analysis manager as each pass runs and potentially
     // invalidates analyses.
     AM.invalidate(L, PassPA);
 
     // Finally, we intersect the final preserved analyses to compute the
     // aggregate preserved set for this pass manager.
     PA.intersect(std::move(PassPA));
 

llvm/test/Feature/optnone-opt.ll

while.cond:                                       ; preds = %while.body, %entry
   store i32 %dec, i32* %x.addr, align 4
   %tobool = icmp ne i32 %0, 0
   br i1 %tobool, label %while.body, label %while.end
 
 while.body:                                       ; preds = %while.cond
   br label %while.cond
 
 while.end:                                        ; preds = %while.cond
-  ret i32 0
+  ret i32 %dec
 }
 
 attributes #0 = { optnone noinline }
 
 ; Nothing that runs at -O0 gets skipped.
 ; O0-NOT: Skipping pass
 ; NPM-O0-NOT: Skipping pass