This is an archive of the discontinued LLVM Phabricator instance.

[NewPM] Support optnone under new pass manager
ClosedPublic

Authored by aeubanks on Jul 9 2020, 3:52 PM.

Details

Summary

This uses pass instrumentation callbacks to skip optional passes.
PassInfoMixin now declares that passes inheriting from it are by default
optional. Using RequiredPassInfoMixin overrides the pass to be required.
The new OptNoneInstrumentation is part of StandardInstrumentations.

The feature of skipping optional passes for optnone functions under NPM
is gated on a -enable-npm-optnone flag. Currently it is by default
false. That is because we still need to mark all required passes to be
required. Otherwise optnone functions will start behaving incorrectly.
After that is done in following changes, we can remove the flag and
always enable this.

All adaptors/managers must be required, since the pass(es) they are
wrapping may be required.

In the future, opt-bisect will use this same mechanmism of determining
which passes are required/optional.

Depends on D83498.

Diff Detail

Event Timeline

aeubanks created this revision.Jul 9 2020, 3:52 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
ychen added inline comments.Jul 9 2020, 5:11 PM
llvm/include/llvm/IR/PassInstrumentation.h
150 ↗(On Diff #276860)

Could we do this to not changing the callback API?
ShouldRun &= C(Pass.name(), llvm::Any(&IR)) || Pass.isRequired();

aeubanks marked an inline comment as done.Jul 9 2020, 5:28 PM
aeubanks added inline comments.
llvm/include/llvm/IR/PassInstrumentation.h
150 ↗(On Diff #276860)

Each pass instrumentation should decide whether or not to run the pass based on whether or not the pass is required or optional. An optional pass may still be run, (which should be the case for the vast majority of instances).

For example, the optnone would only care if a pass is required or not if it sees that a function is marked optnone.
Similarly, opt-bisect would only care if a pass is required if it's hit the bisect limit.

aeubanks marked an inline comment as done.Jul 9 2020, 6:43 PM
aeubanks added inline comments.
llvm/include/llvm/IR/PassInstrumentation.h
150 ↗(On Diff #276860)

Sorry, now I understand what you mean, the ands and ors confused me.

I don't want to rule out the possibility of some future pass instrumentation wanting to skip even a required pass. But I am open to discussion on this point.

ychen added a comment.Jul 10 2020, 9:21 AM

High-level request: how about split this patch into two, the first for the require pass part; the second for the PassInstrument callback. Then we could discuss the choices of first patch and D82344.

llvm/include/llvm/IR/PassInstrumentation.h
150 ↗(On Diff #276860)

I don't want to rule out the possibility of some future pass instrumentation wanting to skip even a required pass. But I am open to discussion on this point.

That makes sense. However, since this requires changing the callback API(return value or parameter), and there is no real use of it for the moment. IMHO we should defer it to the use case comes up. If there was no change to the callback API, I wouldn't mind doing this for now.

The immediate motivation for the require is the same as D82344 (the approach is a little bit different). That's we don't want to consider infrastructure passes (pass managers, adaptor passes that have a nested pass manager)

aeubanks marked an inline comment as done.Jul 10 2020, 9:30 AM

High-level request: how about split this patch into two, the first for the require pass part; the second for the PassInstrument callback. Then we could discuss the choices of first patch and D82344.

Good idea, will split the patch and take a closer look at your patch.

llvm/include/llvm/IR/PassInstrumentation.h
150 ↗(On Diff #276860)

Sounds good, I'll go with your approach for keeping the current API.

High-level request: how about split this patch into two, the first for the require pass part; the second for the PassInstrument callback. Then we could discuss the choices of first patch and D82344.

Good idea, will split the patch and take a closer look at your patch.

I split the required passes part into https://reviews.llvm.org/D83575.

High-level request: how about split this patch into two, the first for the require pass part; the second for the PassInstrument callback. Then we could discuss the choices of first patch and D82344.

Good idea, will split the patch and take a closer look at your patch.

I split the required passes part into https://reviews.llvm.org/D83575.

Could you remove the Required part of this patch?

aeubanks updated this revision to Diff 278871.Jul 17 2020, 11:38 AM

Remove required passes part
Rebase

aeubanks edited reviewers, added: hans; removed: bollu.Jul 17 2020, 11:39 AM

High-level request: how about split this patch into two, the first for the require pass part; the second for the PassInstrument callback. Then we could discuss the choices of first patch and D82344.

Good idea, will split the patch and take a closer look at your patch.

I split the required passes part into https://reviews.llvm.org/D83575.

Could you remove the Required part of this patch?

Done. The test now fails because your patch for required passes hasn't been submitted yet, after that's submitted this one should be good to go.

ychen added inline comments.Jul 18 2020, 11:40 PM
llvm/include/llvm/Passes/StandardInstrumentations.h
57

I may probably name this SkipPassInstrumentation. But it is up to you.

65

How about calling this DebugLogging?

llvm/lib/Passes/StandardInstrumentations.cpp
27

Why need this?

270
if (F && F->hasOptNone()) {
   ...
}
return true;
aeubanks updated this revision to Diff 279313.Jul 20 2020, 11:06 AM
aeubanks marked 4 inline comments as done.

Address comments

ychen accepted this revision.Jul 20 2020, 6:50 PM

LGTM with one nit.

llvm/include/llvm/Passes/StandardInstrumentations.h
57

Errr, let's go with OptNoneInstrumentation if it's your preference. This is a bit too verbose IMHO.

This revision is now accepted and ready to land.Jul 20 2020, 6:50 PM
aeubanks updated this revision to Diff 279563.Jul 21 2020, 9:52 AM

Rename OptNoneInstrumentation

This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.
llvm/include/llvm/Passes/StandardInstrumentations.h
77

(No need to change) PrintIR & TimePasses don't need to appear in the initializer list. They would be default initialized.