This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Python][Linalg] Add missing attributes to linalg ops
ClosedPublic

Authored by nicolasvasilache on Mar 26 2021, 11:01 AM.

Details

Summary

This revision tightens up the handling of attributes for both named
and generic linalg ops.
To demonstrate the IR validity, a working e2e Linalg example is added.

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Mar 26 2021, 11:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2021, 11:01 AM
mlir/test/Bindings/Python/dialects/linalg/opsrun.py
27

Oof. What is going on with the body not populating?

(Side note, you can just say str(module))

mlir/test/Bindings/Python/dialects/linalg/opsrun.py
27

Well I did not hook it up yet so there is not magic :) it will come.
I'm looking for the fastest DFS to execution first (at least in the ongoing hacks).

Make example work.

nicolasvasilache retitled this revision from [mlir][NOTFORCOMMIT] Ongoing hacks to [mlir][Linalg][Python] Working e2e Linalg example in python.Mar 29 2021, 8:57 AM
nicolasvasilache edited the summary of this revision. (Show Details)
nicolasvasilache added a reviewer: shabalin.
nicolasvasilache retitled this revision from [mlir][Linalg][Python] Working e2e Linalg example in python to [mlir][Python][Linalg] Add attributes to named ops.Mar 31 2021, 8:06 AM
nicolasvasilache edited the summary of this revision. (Show Details)
nicolasvasilache retitled this revision from [mlir][Python][Linalg] Add attributes to named ops to [mlir][Python][Linalg] Add missing attributes to linalg ops.Mar 31 2021, 8:12 AM

Fix test and rebase.

Harbormaster completed remote builds in B96535: Diff 334446.
ftynse added inline comments.Mar 31 2021, 9:34 AM
mlir/include/mlir-c/AffineMap.h
181 ↗(On Diff #334448)

How about the first argument being void * instead? This will allow the caller to pass in any container they want, not necessarily a contiguous, and also additional data if desired.

mlir/lib/Bindings/Python/IRAffine.cpp
547–549 ↗(On Diff #334448)

Nit: name shadowing almost got me here with compressed used twice

mehdi_amini added inline comments.Mar 31 2021, 9:54 AM
mlir/test/Bindings/Python/dialects/linalg/opsrun.py
55

Can't all the convert- be inside the func(...) group?

nicolasvasilache marked 3 inline comments as done.

Address comments.

nicolasvasilache marked an inline comment as done.Apr 1 2021, 12:33 AM
nicolasvasilache added inline comments.
mlir/test/Bindings/Python/dialects/linalg/opsrun.py
55

nope, updated to maximally put stuff under func, the rest is module.

ftynse accepted this revision.Apr 1 2021, 1:13 AM
ftynse added inline comments.
mlir/include/mlir-c/AffineMap.h
181 ↗(On Diff #334448)

MlirAffineMap *result should also become void *. The simplest user will just cast.

This revision is now accepted and ready to land.Apr 1 2021, 1:13 AM
nicolasvasilache marked an inline comment as done.

Address comment.

nicolasvasilache marked an inline comment as done.Apr 1 2021, 1:17 AM
nicolasvasilache added inline comments.
mlir/include/mlir-c/AffineMap.h
181 ↗(On Diff #334448)

missed it, thanks!

This revision was landed with ongoing or failed builds.Apr 1 2021, 1:21 AM
This revision was automatically updated to reflect the committed changes.
nicolasvasilache marked an inline comment as done.

It doesn't look like any C-API or Python API unit tests of the new API were added? Would you mind adding something basic there?

It doesn't look like any C-API or Python API unit tests of the new API were added? Would you mind adding something basic there?

Landed in 48268aa0a9c18ab244b252b9617b91f6f5055c95.