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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
