Page MenuHomePhabricator

[mlir][Linalg] Allow calling named ops when available and make it the default.
ClosedPublic

Authored by nicolasvasilache on Fri, Mar 26, 8:28 AM.

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Fri, Mar 26, 8:28 AM

Sweet!

mlir/lib/Bindings/Python/mlir/dialects/linalg/__init__.py
24

Strictly speaking, PyRepr -> YAML does not happen at compiler time at the moment.

52

Thanks for the effort to put an extensive doc!

stellaraccident accepted this revision.Fri, Mar 26, 6:42 PM

Nice! I think this needs a fair bit more iteration to get it all laid out right, but I like that everything is now unified. We can iterate from here.

mlir/lib/Bindings/Python/mlir/dialects/linalg/__init__.py
6

Strictly speaking, these are the backing OpView *classes* generated from the linalg tablegen definitions, including things like linalg.generic and those imported via the DSL/YAML path. These will all be named like GenericOp, MatmulOp, CopyOp, etc. They can be used to either construct the corresponding op in the IR, or during traversal, instances of these op classes will be returned.

21

Let's be consistent on named. "... then turn into the core C++ Op classes and Python OpView classes."

48

Note to self: once this lands, I want to look at all of the namespaces and terminology and see how we can better make it consistent. I suspect some extra explicitness in the core_named_ops versions (i.e. delineating them as part of the linalg named op namespace will let us remove this warning.

mlir/lib/Bindings/Python/mlir/dialects/linalg/opdsl/lang/dsl.py
45

This should already be populated as self.model.cpp_op_name (which should likely be renamed cpp_class_name and be explicitly configurable at op definition time (right now it is done with some string/case munging).

69

Something like from .... import linalg as linalg_ops. (may have mis-counted number of dots). Agreed this should just be client in the parent namespace.

I am still vaguely trying to keep the namespace relocatable.

70

Nit: Prefer to add outer parens so it can reflow vs a line continuation \.

mlir/lib/Bindings/Python/mlir/dialects/linalg/opdsl/lang/emitter.py
53

Note to self: I almost factored this as a class in anticipation of needing to carry a bunch of state like this but wanted to see how bad it was. Now I see and want to clean this up.

mlir/test/Bindings/Python/dialects/linalg/ops.py
79

Can you add a TODO here: I believe this will be created without its region populated whereas using the standalone builder function below will populate it. I can unify that.

97

Note to self: This reads more like named "standalone builder functions" which then return an instance of the corresponding OpView class. I expect that if the mechanism is working, a further check on isinstance(created_op, linalg.MatmulOp) should return True. Want to verify that is working and then document this correspondance between standalone builder methods and OpView classes.

This revision is now accepted and ready to land.Fri, Mar 26, 6:42 PM
nicolasvasilache marked 11 inline comments as done.Mon, Mar 29, 6:21 AM
nicolasvasilache added inline comments.
mlir/lib/Bindings/Python/mlir/dialects/linalg/__init__.py
6

Not 100% sure how to phrase and incorporate all this, please improve and land as you see fit.

48

ack

52

ack

mlir/lib/Bindings/Python/mlir/dialects/linalg/opdsl/lang/dsl.py
69

thanks!

mlir/lib/Bindings/Python/mlir/dialects/linalg/opdsl/lang/emitter.py
53

ack, letting you do it as you see fit :)

mlir/test/Bindings/Python/dialects/linalg/ops.py
79

yes, it's def. broken atm, will try to fix in the n+2 commit once the n+1 commit executes properly, unless you get to it first.

97

ack, that would be nice, leaving to you as a followup.