Page MenuHomePhabricator

allow custom OptBisect classes set to LLVMContext
ClosedPublic

Authored by yrouban on Mar 14 2018, 5:07 AM.

Details

Summary

This patch introduces a way to set custom OptPassGate instances to LLVMContext.
A new instance field OptBisector and a new method setOptBisect() are added to the LLVMContext classes. These changes allow to set a custom OptBisect class that can make its own decisions on skipping optional passes. Another important feature of this change is ability to set different instances of OptPassGate to different LLVMContexts. So the different contexts can be used independently in several compiling threads of one process.
One test is added.

Diff Detail

Repository
rL LLVM

Event Timeline

yrouban created this revision.Mar 14 2018, 5:07 AM
Eugene.Zelenko added inline comments.Mar 14 2018, 7:00 AM
include/llvm/IR/OptBisect.h
39 ↗(On Diff #138330)

Please use = default;

lib/IR/LLVMContext.cpp
338 ↗(On Diff #138330)

Please separate with empty line.

lib/IR/LLVMContextImpl.cpp
43 ↗(On Diff #138330)

Please use default initialization instead.

lib/IR/LLVMContextImpl.h
1358 ↗(On Diff #138330)

Please use default initialization, = nullptr

yrouban marked 4 inline comments as done.Mar 14 2018, 9:00 AM
yrouban updated this revision to Diff 138384.Mar 14 2018, 9:02 AM

fixed according to all Eugene's comments

vsk added a comment.Mar 14 2018, 9:38 AM

Thanks for the patch. Could you share a link to some motivating bug or use case which requires a custom bisector? I want to rule out there being any simpler ways to do the same thing.

As for this patch, I think it would help to have a test. You might do this by surfacing a dummy bisector through opt.

I have some concerns about this patch, mostly centered around what the intended use cases are.

Compiling in multiple threads probably isn't handled well with the current implementation (i.e. prior to this patch) but having different instances of the OptBisect object for each thread may not fix it. For the opt bisect process to work, individual optimizations need to call OptBisect::checkPass() in a deterministic order. I would think it's probably best to just avoid multi-threaded compilation when using OptBisect.

Maybe if I knew what is motivating this change it would make more sense to me.

Maybe if I knew what is motivating this change it would make more sense to me.

For our custom JIT compiler pipeline we want to have a flexible control on pipeline behavior,
which might have a number of different applications. Immediate one is the ability to gracefully
shut down the compilation that takes too long.
Most likely this particular need is completely off the roster of a static compiler though.

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.

Compiling in multiple threads probably isn't handled well with the current implementation (i.e. prior to this patch) but having different instances of the OptBisect object for each thread may not fix it.

Btw, we do compile in multiple threads, having separate LLVMContexts for each thread.

yrouban updated this revision to Diff 138514.Mar 15 2018, 3:16 AM

added a simple unit test to LegacyPassManagerTest

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. Perhaps we can generalize the skip mechanism and allow a way to add other callbacks. There is already some code in place for function passes to check for the "opt_none" attribute in addition to calling OptBisect.

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. 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.

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?

Is it a matter of properly wording commit message?

I think that the class name OptBisect is a bit misleading. The word Bisect explains just one way of using the skip functionality. I would change to something like OptPassGate, meaning that the class just decides if the pass should be skipped or run.

Is it a matter of properly wording commit message?

I think that the class name OptBisect is a bit misleading. The word Bisect explains just one way of using the skip functionality. I would change to something like OptPassGate, meaning that the class just decides if the pass should be skipped or run.

This suggestion is kinda in line with my first suggestion on generalization - introduce a base class OptPassGate, and derive OptBisect from it; routines that skip the passes should operate with OptPassGate, not with OptBisect directly.

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.

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.

I can try to make some time to think more about this soon and propose something ...

Andrew, thank you for sharing your design thoughts on OptBisect. But it all sounds long to implement. Do you mind if this patch is landed as is? This patch is not big and we will be able to fix it to your new design when it is ready.

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.

I have seen this suggestion and honestly I dont see how it could be dropped w/o first replacing every invocation of skipPass with a debug-counter's shouldExecute.
Theoretically debug-counters are at least as powerful as OptBisect, but that stays theoretical until somebody really places them into the code.
For now I only see 5 instances of shouldExecute in a whole LLVM tree.

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.

So, it seems that Yevgeny's suggestion on optional OptPassGate:

  • makes current OptBisect implementation strictly better
  • allows us to proceed with our controlling feature
  • does not stop anybody from discussing various facets of a better Bisect design

Hence to me it looks like a win-win (or rather win-no-loss ;) situation.

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.

I'm personally interested in moving new pass manager forward, so I'ld be glad to participate.
Note, that I'm not particularly tied to/experienced with OptBisect feature, so it would be nice if you could start this discussion.

As long as you're OK with the fact that this will probably be replaced in the near future (and please add a comment indicating that above the setOptPassGate function), I'm OK with this being added. I do prefer OptPassGate as a name for the generalized functionality.

yrouban updated this revision to Diff 139574.Mar 23 2018, 4:08 AM
yrouban retitled this revision from OptBisect is improved to be overridden in LLVMContext to allow custom OptBisect classes set to LLVMContext.
yrouban edited the summary of this revision. (Show Details)

Rebased on top of NFC patch D44821.

include/llvm/IR/LLVMContext.h
324 ↗(On Diff #139574)

It seems like a bad idea to pass this in by reference. How is the lifetime of this object going to be managed? I don't think it's correct for the LLVMContext to have a reference to anything outside the LLVMContext. If you're going to pass in an object, I think LLVMContext must take ownership of the object, as happens with LLVMContext::setDiagnosticHandler().

lib/IR/LLVMContextImpl.h
1362 ↗(On Diff #139574)

I missed this in the last review. This function should be const.

yrouban added inline comments.Mar 29 2018, 9:13 AM
include/llvm/IR/LLVMContext.h
324 ↗(On Diff #139574)

Andy, can you please explain why LLVMContext should not have a ref to outside? The OptBisect, for example, is one single global object that is outside of LLVMContext.
What if a user wants to set one OptPassGate to several contexts?
I would suggest that we accept this minimal design and see if there will be any demand for ownership.

lib/IR/LLVMContextImpl.h
1362 ↗(On Diff #139574)

D44821 did not change this. LLVMContextImpl::getOptBisect() used to be non-const.

fedor.sergeev added inline comments.Mar 30 2018, 4:00 AM
lib/IR/LLVMContextImpl.h
1362 ↗(On Diff #139574)

I agree with Andrew, it is natural to have it const.

yrouban marked an inline comment as done.Apr 2 2018, 2:24 AM
yrouban updated this revision to Diff 140611.Apr 2 2018, 2:27 AM

getOptPassGate() has been made const

fedor.sergeev accepted this revision.Apr 2 2018, 11:19 PM

LGTM, but, please, wait for Andrew to ack this.

This revision is now accepted and ready to land.Apr 2 2018, 11:19 PM
include/llvm/IR/LLVMContext.h
324 ↗(On Diff #139574)

My concern is lifetime management. If the LLVMContext receives a reference to an object, the lifetime of that object must be guaranteed to extend as long as the lifetime of the LLVMContext. That was always true with the OptBisect object, but with an arbitrary input from any caller we can't be sure of that. This feels like a weak spot in the interface.

fedor.sergeev added inline comments.Apr 3 2018, 1:25 PM
include/llvm/IR/LLVMContext.h
324 ↗(On Diff #139574)

This is definitely a weak spot in the interface, which can not be addressed without a heavy redesign of the whole approach, which borders pretty much on throwing everything out and coming out with a completely new design.

Will a comment on the interface that explicitly mentions that management of the lifetime is at discretion of interface user satisfy your concerns?

andrew.w.kaylor added inline comments.Apr 3 2018, 1:39 PM
include/llvm/IR/LLVMContext.h
324 ↗(On Diff #139574)

Yes, given the limited expected usage of this interface I would be satisfied with just a comment.

yrouban planned changes to this revision.Apr 3 2018, 7:44 PM
yrouban marked an inline comment as done.

ok. I'm going to update description of the set/get methods.

yrouban updated this revision to Diff 140906.Apr 3 2018, 10:35 PM

changed description of get/set methods.

This revision is now accepted and ready to land.Apr 3 2018, 10:35 PM

a few nits, LGTM as soon as those are corrected.

include/llvm/IR/LLVMContext.h
325 ↗(On Diff #140906)

must be guaranteed by the caller

326 ↗(On Diff #140906)
  1. delete extra 'the'
  2. I dont like 'pass manager' here. LLVMContext is not a property of pass manager. Can we say "used by compilation"?
yrouban updated this revision to Diff 140919.Apr 4 2018, 3:00 AM

fixed descriptions

andrew.w.kaylor accepted this revision.Apr 4 2018, 10:02 AM

LGTM

Thank you for your patience in addressing my concerns.

This revision was automatically updated to reflect the committed changes.