[PATCH] D83519: [NewPM] Support optnone under new pass manager

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks marked an inline comment as done.
aeubanks added a comment.

In D83519#2144403 <https://reviews.llvm.org/D83519#2144403>, @ychen wrote:

> 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 <https://reviews.llvm.org/D82344>.


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



================
Comment at: llvm/include/llvm/IR/PassInstrumentation.h:150
     for (auto &C : Callbacks->BeforePassCallbacks)
-      ShouldRun &= C(Pass.name(), llvm::Any(&IR));
+      ShouldRun &= C(Pass.name(), Pass.isRequired(), llvm::Any(&IR));
     return ShouldRun;
----------------
ychen wrote:
> aeubanks wrote:
> > aeubanks wrote:
> > > ychen wrote:
> > > > Could we do this to not changing the callback API?
> > > > `ShouldRun &= C(Pass.name(), llvm::Any(&IR)) || Pass.isRequired();`
> > > 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.
> > 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.
> > 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)
Sounds good, I'll go with your approach for keeping the current API.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83519/new/

https://reviews.llvm.org/D83519