[PATCH] D44464: OptBisect is improved to be overridden in LLVMContext

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added a comment.

In https://reviews.llvm.org/D44464#1041373, @yrouban wrote:

> In https://reviews.llvm.org/D44464#1039091, @andrew.w.kaylor wrote:
>
> > I would prefer not to overload the behavior of OptBisect. ...
>
>
> Andrew, would it be ok to create an NFC extracting a pure virtual class OptPassGate from OptBisect? Then I could make use of the OptPassGate interface to implement context specific pass gates without referring to OptBisect.


Sorry, I've been distracted with other tasks and haven't gotten back to this.

I have some partially formed ideas about how to generalize this. Basically, we want a general mechanism that will stop optional passes from being run.  I think OptBisect probably ought to be a user of that general mechanism rather than implementing it. With the legacy pass manager, I think it made sense to do things the way we did with putting this into LLVMContext, but it really feels to me like the sort of thing the pass manager should be doing. I don't know if the new pass manager is well suited to that or not.  We originally implemented OptBisect as an opt-in mechanism called by the passes because it was a convenient way to count the passes while allowing the passes themselves to control whether or not they could be skipped.

What I'm getting at here is that since we are on the verge of having to mostly re-implement OptBisect for the new pass manager, I'd like to take the opportunity to re-visit the design and see if it should be done differently. Your use case is a very reasonable thing that we should support in the design.

What you are proposing is a reasonable way to get it working quickly with the existing design, but I'm not convinced that it is the best long-term solution. I do think it's better than having some vector of callbacks as my earlier suggestion implied, but I don't think these are the only two options. Since we have some time before the next release branch point, I don't see a need to rush anything in.

Here are the constraints as I see them:

The pass manager is responsible for scheduling and running passes.
The pass itself knows whether or not it can be skipped.
In some cases, the IR indicates whether or not the pass should be skipped.
In some cases, some external object decides when passes should be skipped.
OptBisect needs a way to count the passes that have not been skipped.
OptBisect needs some basic information about the pass to print a useful status message.

We don't want to build to much into the pass interface, so it might not be possible to involve the new pass manager in the skip decision. When this was originally implemented that was our conclusion, and that might still be the way things turn out. I just want to consider it again. A few people have suggested that OptBisect could be dropped completely in favor of a debug counter. I'm not sure if that makes supporting your use case easier or harder.

The current implementation inserts OptBisect into the decision flow by default, but that's almost never what we want. OptBisect does an early return when it isn't enabled, so that's not terrible, but conceptually it's wrong. A single, optional pass gate object that can be set by the client isn't necessarily a bad idea.

I can try to make some time to think more about this soon and propose something, but if you would like to propose something I'd be happy to consider it. Ideally, the proposal would start from what's needed for the new pass manager and be sent to the LLVM Dev mailing list for discussion.


https://reviews.llvm.org/D44464