Page MenuHomePhabricator

[NewPM][PassInstrument] Add a new kind of before-pass callback that only get called if the pass is not skipped
ClosedPublic

Authored by ychen on Jul 28 2020, 10:33 AM.

Details

Summary

TODO

  • PrintIRInstrumentation and TimePassesHandler would be using this new callback.
  • "Running pass" logging will also be moved to use this callback.

Diff Detail

Event Timeline

ychen created this revision.Jul 28 2020, 10:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2020, 10:33 AM
ychen requested review of this revision.Jul 28 2020, 10:33 AM
asbirlea added inline comments.Jul 28 2020, 10:48 AM
llvm/include/llvm/IR/PassInstrumentation.h
77

Perhaps BeforePassRequiredFunc (or similar) is more informative that these callbacks are run for required passes?

aeubanks added inline comments.Jul 28 2020, 11:03 AM
llvm/include/llvm/IR/PassInstrumentation.h
77

"required" is sort of misleading, these callbacks will run on passes that aren't "required" as long as nothing says the pass should be skipped.
BeforePassIsNotSkippedFunc? a little verbose

asbirlea added inline comments.Jul 28 2020, 11:07 AM
llvm/include/llvm/IR/PassInstrumentation.h
77

Sure, in my mind if a pass is not skipped, it can be viewed as required. I'm ok with either renaming as long we make it more informative.

ahh, naming is hard. I was thinking of required to run. Yes, it could also be required pass which is misleading. I'll go with BeforePassIsNotSkippedFunc which is not ambiguous.

ychen updated this revision to Diff 281316.Jul 28 2020, 11:54 AM
ychen edited the summary of this revision. (Show Details)
  • Use BeforeNonSkippedPassFunc

What about unit tests?

llvm/include/llvm/IR/PassInstrumentation.h
71

Rather than this sentence, I think there should be a comment that we call all BeforePassFuncs to determine if the pass should run or not.

ychen updated this revision to Diff 281392.Jul 28 2020, 3:58 PM
ychen marked an inline comment as done.
  • Add unit tests
  • Add comments
This revision is now accepted and ready to land.Jul 28 2020, 4:04 PM
This revision was landed with ongoing or failed builds.Jul 29 2020, 8:27 AM
This revision was automatically updated to reflect the committed changes.
ychen marked an inline comment as not done.
yrouban added inline comments.
llvm/include/llvm/IR/PassInstrumentation.h
76–77

I would rename BeforePass to ShouldRun.
then BeforeNonSkippedPass to BeforePass.