This is an archive of the discontinued LLVM Phabricator instance.

[NewPM] Only verify loop for nonskipped user loop pass
ClosedPublic

Authored by ychen on Jul 30 2020, 12:47 PM.

Details

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.

Diff Detail

Event Timeline

ychen created this revision.Jul 30 2020, 12:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2020, 12:47 PM
ychen requested review of this revision.Jul 30 2020, 12:47 PM
ychen added inline comments.Jul 30 2020, 12:48 PM
llvm/test/Feature/optnone-opt.ll
37

This is to activate LCSSA analysis.

This revision is now accepted and ready to land.Jul 30 2020, 5:28 PM

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();

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 added inline comments.Jul 31 2020, 4:29 AM
llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
315

Is that tabs?

ychen updated this revision to Diff 282246.Jul 31 2020, 9:22 AM
  • 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 added a comment.Jul 31 2020, 9:35 AM

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 planned changes to this revision.Jul 31 2020, 10:30 AM

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

ychen updated this revision to Diff 282262.Jul 31 2020, 10:38 AM
  • Checking Callback pointer before use
This revision is now accepted and ready to land.Jul 31 2020, 10:38 AM

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.

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 added a comment.Jul 31 2020, 3:39 PM

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).

aeubanks accepted this revision.Jul 31 2020, 3:47 PM