This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Revisit 0-D abstraction
ClosedPublic

Authored by nicolasvasilache on Mar 8 2020, 12:14 PM.

Details

Summary

This revision takes advantage of the empty AffineMap to specify the
0-D edge case. This allows removing a bunch of annoying corner cases
that ended up impacting users of Linalg.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
rriddle added inline comments.Mar 8 2020, 12:30 PM
mlir/lib/IR/AffineMap.cpp
328

Do we need these asserts? None of the other methods assert this.

353

Same here.

rriddle accepted this revision.Mar 8 2020, 12:31 PM
This revision is now accepted and ready to land.Mar 8 2020, 12:31 PM

Remove unnecessary asserts.

pifon2a accepted this revision.Mar 8 2020, 1:28 PM

Awesome! Thank you for doing this, @nicolasvasilache!

mravishankar accepted this revision.Mar 9 2020, 12:42 AM

This is awesome, but this is a breaking change to clients who had implemented the WAR. I can help fix the IREE changes.

herhut accepted this revision.Mar 9 2020, 2:31 AM

Thanks, much cleaner solution!

ok since everyone seems to be onboard and pifon@ is integrate duty this week I'll coordinate with him.
Thanks for your quick turnaround!

herhut added a comment.Mar 9 2020, 6:02 AM

ok since everyone seems to be onboard and pifon@ is integrate duty this week I'll coordinate with him.

pifon@ is next week.

mravishankar requested changes to this revision.Mar 10 2020, 12:23 AM

I looked at this change a little bit deeper, and I am not sure this works. This seems to have an affine_map for scalars being () -> (). This seems odd. Isn't it better to have a scalar access be (i,j) -> (), or something like that.
That needs change to the AffineMap constructor above.

This revision now requires changes to proceed.Mar 10 2020, 12:23 AM

This seems to have an affine_map for scalars being () -> (). This seems odd. Isn't it better to have a scalar access be (i,j) -> (), or something like that.

Whatever we do for 0-D is a convention, we can chose the convention we want.
()->() did not require parser changes whereas (i, j) -> () does.

I went ahead and implemented those changes.

Address comments.

bondhugula requested changes to this revision.Mar 10 2020, 7:21 AM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/include/mlir/IR/AffineMap.h
49 ↗(On Diff #249358)

We could just go for for:

static AffineMap get(unsigned dimCount, unsigned symCount, MLIRContext *context);

In any case, please have context appear last to be consistent with everything else.

This revision now requires changes to proceed.Mar 10 2020, 7:21 AM
bondhugula added inline comments.Mar 10 2020, 7:29 AM
mlir/include/mlir/IR/AffineMap.h
47 ↗(On Diff #249358)

dim -> dimensions

mlir/lib/IR/AffineMap.cpp
367

/*numSymbols=*/0

367

How do you know whether results is non-empty here?

Address review.

mravishankar accepted this revision.Mar 10 2020, 12:10 PM

Address comments.

nicolasvasilache marked 7 inline comments as done.Mar 10 2020, 12:15 PM
nicolasvasilache added inline comments.
mlir/lib/IR/AffineMap.cpp
367

Good point, thanks Uday!

This revision was not accepted when it landed; it landed in state Needs Review.Mar 10 2020, 12:34 PM
This revision was automatically updated to reflect the committed changes.
nicolasvasilache marked an inline comment as done.

Very cool to see this happen! This will allow me to pay off some engineering debt for vector.contract as well (where I was forced to use empty maps rather than (i,j) -> () maps).