[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

307–311

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

315

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/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) {
 
     do {
       Loop *L = Worklist.pop_back_val();
 
       // Reset the update structure for this loop.
       Updater.CurrentL = L;
       Updater.SkipCurrentLoop = false;
 
+      // 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;
+
 #ifndef NDEBUG
+      if (!isSpecialPass(Pass.name(), {"PassManager"})) {
         // 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.

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