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

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev added a comment.

In https://reviews.llvm.org/D44464#1039091, @andrew.w.kaylor wrote:

> In https://reviews.llvm.org/D44464#1037846, @fedor.sergeev wrote:
>
> > I would like to stress that we consider OptBisect to be more than just "bisector", but rather the only
> >  available way of skipping optimization passes with an arbitrarily complex control pattern.
>
>
> I would prefer not to overload the behavior of OptBisect. It's meant to be a simple mechanism that does just this one thing.


Agreed. And this one thing is - "skip the pass".
So in this sense I do not believe we overload the behavior.

> Perhaps we can generalize the skip mechanism and allow a way to add other callbacks.

I'm not sure that skip mechanism needs multiple independent callbacks trying to work together...

I see two ways to generalize:

- introduce a base class PassSkipper, derive OptBisect from it and otherwise do pretty much what this fix does (i.e. install a single PassSkipper impl into LLVMContext)
- implement a vector of callbacks, query this vector of callbacks in all those places where it currently does OptBisect.shouldRunPass; install current OptBisect as one of those callbacks by default

Which one of these would you consider a better solution?

> There is already some code in place for function passes to check for the "opt_none" attribute in addition to calling OptBisect.

opt_none is different as it is a specific opt-in from each pass whether it considers itself to be of "opt" nature or not.

> With regard to the multithreading issue, I can see how that would work if you are exercising specific control over compilation and you know that each thread is working on an independent job.

Thats right. And with this particular organization majority of LLVM just works. Not OptBisect though...

> However, in the more general case where multiple threads are working on a single compilation unit and tasks are assigned to threads as the threads are available, this won't be useful for OptBisect.
>  I'm not aware of anything that currently compiles in this way, but it's something that has been talked about.
>  My concern here is that I want to avoid giving the impression that OptBisect could support the second case.

Is it a matter of properly wording commit message?


https://reviews.llvm.org/D44464