New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New compiler. #4513
New compiler. #4513
Conversation
As title.
767f2cf
to
3090fe7
Compare
As title
I just tested this PR and everything seems good. It actually exposed a problem in my pipeline since there was a name conflict in stage function names. Also, the "print after" feature is great! |
class CompilerPass(object): | ||
|
||
@abstractmethod | ||
def __init__(self, *args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does __init__
has to be an abstractmethod
? A lot of times it's just a passthru to the parent's implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it needs to be. I tried a bunch of different ways of describing this class and the pipeline and I think that this is an artifact from when pass registration required an instance of a pass to be registered. I was thinking about how pass classes might be extended, and was considering the merits of inheritance over composition and settled for the latter as most of the time passes are pretty unique should be state free by design.
I just tried adapting the literal dispatch PR to use the new pass API. It's so easy. |
As title.
As title
The latest commit is causing 28 (out of 47) errors in |
Thanks @sklam, turns out that removing the |
break | ||
else: | ||
raise ValueError("Could not find pass %s" % location) | ||
self.passes.insert(idx + 1, (pass_cls, str(pass_cls))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the str(pass_cls)
be from a description
parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be, I was maintaining the original interface style, is a description useful in this new style as passes are uniquely defined and have a name? Am happy to do either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same pass could be added several times; i.e. a pass can require a DCE beforehand. A description can help inform why the pass is added. Not high priority though.
|
||
def finalize(self): | ||
""" | ||
Finalize the PassManager, after which no more passes may be added and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no more passes may be added
^ Not enforced?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was in two minds... On the one hand, it's quite useful to be able to say "finalize this, compute analysis usage etc" and that be fixed. On the other, its quite useful to be able to mutate a "finalized" pass, which unsets its finalized state and subsequently re-finalize it when done. I think one way of achieving the latter with finalization equating to immutable would be to have a copy constructor or similar to generate a new pipeline from an existing? Suggestions welcomed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea on having a copy constructor
Conflicts: numba/ir_utils.py
Looks to me that the self-reference is only needed for pipeline re-entrant. |
yeah, I think that's the case. |
As title
As title
As title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch. This is an important refactor to cleanup the compiler pipeline. Other comments in the review can be resolved later. We'll learn more once we use the new code in practice.
This PR is large, both in terms of churn and in terms of conceptual change.
The intent is as follows:
Redesign the Numba pipeline to be a bit more like that which is in LLVM. This
to comprise:
PassManager
class are used to orchestrate a "pipeline" ofpasses to execute.
CompilerBase
class instance holds the state, thePassManager
orchestrated passes operate on this state.
DefaultPassBuilder
class provides static methods defining default orcommonly used pipelines.
In addition, points for future work on safety and optimisation are added.
AnalysisUsage
cf. that in LLVM is stubbed out to permit eventualdeclaration of analysis dependencies between passes.
have been made.
whether they are analysis only passes.
for a given pipeline execution.
print_after
like functionality to print the IRfollowing any transform is added.
The approach taken in this PR:
compiler pass.
are untyped (i.e. do not need type information and run before type inference)
and those which are typed (i.e. need type information and run after type
inference). These can be found in
numba.untyped_passes
andnumba.typed_passes
respectively. Passes relating object mode are innumba.object_mode_passes
.found in
numba.compiler_machinery
. This includes thePassManager
, thebase class for passes
CompilerPass
, and thepass_registry
decorator (usedfor registering new passes).
compiler.py
has been heavily refactored to make use of themore modular design described above. Most notably the
CompilerBase
class isnow responsible for determining how to execute pipelines defined by
PassManager
instances with eachPassManager
holding a single pipeline.The compilation state information is also attached to the
CompilerBase
class and is not part of a
PassManager
. This makes it easier for usersbuilding their own compilers to not have to subscribe to the Numba JIT
compilation semantics, the
Compiler
state can be user defined as can thepipeline managed by the
PassManager
itself (along with all the passes andtheir execution order).
accommodate the above.
Outstanding items/areas of concern:
CompilerPass
design and its registration mechanism reasonable?more configuration from the same code. But equally inheritance/composition
could achieve the same.
getting up to speed quickly (along with new docs/examples).
"mode" based pipeline as default? It seems like it could with separate
compiler instances extending from
CompilerBase
each defining a singlepipeline via the
define_pipelines()
method with new named pipelines foreach purpose added to the
DefaultPassBuilder
.Work left on this PR: