Skip to content
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.

Sign up for GitHub

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

[MLIR][Linalg] Expose linalg.matmul and linalg.contract via Python API #126377

Merged
merged 11 commits into from
Feb 10, 2025
Merged

[MLIR][Linalg] Expose linalg.matmul and linalg.contract via Python API #126377

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e3f2a7c
[MLIR][Linalg] Expose linalg.matmul and linalg.contract via Python API
rolfmorel Feb 8, 2025
57b2509
Fix linalg.matmul tests which encoded different placements of indexin…
rolfmorel Feb 8, 2025
18ff3a6
Python formatting
rolfmorel Feb 8, 2025
2e6bf66
Make indexing_maps optional on matmul, as it should be
rolfmorel Feb 8, 2025
85426f6
Fix matmul's indexing_maps issue
rolfmorel Feb 8, 2025
e98838d
Get rid of "whole new type" by introducing "context dependent" defaul…
rolfmorel Feb 9, 2025
931b473
Move builderCall parameter from custom attr class to DefaultValued(Op…
rolfmorel Feb 9, 2025
538566a
Remove junk
rolfmorel Feb 9, 2025
c81c97f
Allow access to `$_builder` in Attr's `defaultValue`
rolfmorel Feb 9, 2025
f83f7ae
Switch argument format to that of OpDSL-derived linalg ops
rolfmorel Feb 10, 2025
7a5234f
Py formatting fix
rolfmorel Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,22 @@ def BroadcastOp : LinalgStructuredBase_Op<"broadcast", [
// Op definition for MatmulOp
//===----------------------------------------------------------------------===//


// DONOTMERGE(rolfmorel): explain why the below is necessary
def DefaultValuedMatmulIndexingMapsAttr :
Attr<AffineMapArrayAttr.predicate, AffineMapArrayAttr.summary> {
let storageType = AffineMapArrayAttr.storageType;
let returnType = AffineMapArrayAttr.returnType;
let convertFromStorage = AffineMapArrayAttr.convertFromStorage;
let constBuilderCall = "$_builder.getAffineMapArrayAttr($0.empty() ? MatmulOp::getDefaultIndexingMaps($_builder.getContext()) : $0)";
let defaultValue = "SmallVector<AffineMap>()";
let valueType = AffineMapArrayAttr.valueType;
let isOptional = 1;

let baseAttr = AffineMapArrayAttr;
}


def MatmulOp : LinalgStructuredBase_Op<"matmul", [
AttrSizedOperandSegments,
LinalgContractionOpInterface]> {
Expand Down Expand Up @@ -606,7 +622,7 @@ def MatmulOp : LinalgStructuredBase_Op<"matmul", [
let arguments = (ins
Variadic<AnyType>:$inputs,
Variadic<AnyShaped>:$outputs,
DefaultValuedOptionalAttr<AffineMapArrayAttr, "{}">:$indexing_maps,
DefaultValuedMatmulIndexingMapsAttr:$indexing_maps, // DONOTMERGE(rolfmorel): explain why this is necessary
Copy link
Contributor

@makslevental makslevental Feb 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get what you're going for here but the right way to do this is just add a custom builder that constructs this attribute rather than adding a whole new type. That won't work for python because python can only call the default builder (which is maybe why you went this direction) but there it's the same story - the MatmulOp builder just needs to construct the correct indexing maps.

Copy link
Contributor Author

@rolfmorel rolfmorel Feb 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, as far as I understand now no builder (really no op-specific C++ code at all) is called when an op gets constructed from Python (see from here to here to here to here to finally here ). The only thing one can do is pass in default values for attributes. Could you point me to an example of specifying "the default builder" for an op?

And yes, preferably I would just have DefaultValuedOptionalAttr<AffineMapArrayAttr, "MatmulOp::getDefaultIndexingMaps($_builder.getContext())">:$indexing_maps in MatmulOp's arguments (note, to my knowledge, we cannot specify custom "builders" in this list), but that doesn't work. The problem is that DefaultValuedOptionalAttr assumes that the defaultValue can be given without access to the context. However, that is not true in this case (that is, attempting the above leads to error: use of undeclared identifier '$_builder'). The workaround is to just expand what DefaultValuedOptionalAttr does to get DefaultValuedMatmulIndexingMapsAttr in which we can modify constBuilderCall so that we can pass the context in case of the defaultValue. That the resulting attr is not "anonymous" - and hence we get a whole new type attr - is rather unfortunate though.

If you have suggestions for how to effect a cleaner approach, I am all ears!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, latest commit gets rid of whole new type: now no other code is needed/aware of that we aren't using DefaultValuedOptionalAttr<AffineMapArrayAttr, ...>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest commit now moves the builderCall parameter to DefaultValued(Optional)Attr.

Probably an even cleaner solution is to just allow $_builder in Attr's defaultValue... (that's an exploration for another time though).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, preferably I would just have DefaultValuedOptionalAttr<AffineMapArrayAttr, "MatmulOp::getDefaultIndexingMaps($_builder.getContext())">:$indexing_maps in MatmulOp's arguments (note, to my knowledge, we cannot specify custom "builders" in this list), but that doesn't work.

Rather than let my dreams be dreams, I decided to just get this to work. I doubt we will get to a cleaner solution than this.

Nevertheless, @makslevental, if you could give some pointers to info on "default builders" and how Python is supposed to invoke those, that would be much appreciated!

DefaultValuedOptionalAttr<TypeFnAttr, "TypeFn::cast_signed">:$cast
);
let results = (outs Variadic<AnyRankedTensor>:$result_tensors);
Expand Down
5 changes: 5 additions & 0 deletions mlir/python/mlir/dialects/linalg/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,11 @@ def __init__(
generic = region_op(GenericOp_, terminator=YieldOp)


@register_attribute_builder("DefaultValuedMatmulIndexingMapsAttr")
def _DefaultValuedMatmulIndexingMapsAttr(x, context):
return ArrayAttr.get([AffineMapAttr.get(v) for v in x])


def matmul(
inputs: Sequence[Union[Operation, OpView, Value]],
*,
Expand Down
4 changes: 1 addition & 3 deletions mlir/test/python/dialects/linalg/ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,9 +337,7 @@ def matmul_op(A, Amem, B, Bmem, Btransposed, Btransposedmem, C, Cmem):
)
linalg.fill_builtin_region(res.operation)
# CHECK: linalg.matmul ins(%[[Amem]], %[[Bmem]] : memref<4x8xf32>, memref<8x12xf32>) outs(%[[Cmem]] : memref<4x12xf32>)
linalg.matmul(
(Amem, Bmem), outs=(Cmem,)
)
linalg.matmul((Amem, Bmem), outs=(Cmem,))

# CHECK: linalg.matmul indexing_maps = [#[[$A_MAP]], #[[$BTrans_MAP]], #[[$C_MAP]]] ins(%[[Amem]], %[[BTransmem]] : memref<4x8xf32>, memref<12x8xf32>) outs(%[[Cmem]] : memref<4x12xf32>)
res = linalg.MatmulOp(
Expand Down