This is an archive of the discontinued LLVM Phabricator instance.

[mlir][WIP] symbol_source attr in linalg.generic proposal
AbandonedPublic

Authored by limo1996 on Jul 8 2020, 3:19 AM.

Details

Reviewers
nicolasvasilache
Summary

Depends On D83191

Diff Detail

Event Timeline

limo1996 created this revision.Jul 8 2020, 3:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2020, 3:19 AM
limo1996 updated this revision to Diff 276367.Jul 8 2020, 4:15 AM

Linting message fixed

ftynse added a subscriber: ftynse.Jul 8 2020, 5:35 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
522 ↗(On Diff #276367)

I think llvm::Optional<int> is preferable to returning implicit -1 as a sign of the attribute being absent.

mlir/lib/Dialect/Linalg/Transforms/DropUnitDims.cpp
321 ↗(On Diff #276367)

Do you see a good reason why this code (and below) is using the default autogenerated builder form instead of the custom builder form? Maybe it is worth updating so that next modifications of the Op arguments don't require changes.

mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
529

We shouldn't be having this attribute on the IndexedGenericOp.

mlir/test/Dialect/Linalg/invalid.mlir
124 ↗(On Diff #276367)

There's no need to check autogenerated error messages

limo1996 marked 2 inline comments as done.Jul 8 2020, 6:01 AM
limo1996 added inline comments.
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
522 ↗(On Diff #276367)

Ahh Thank you. Didn't know about this option.

mlir/test/Dialect/Linalg/invalid.mlir
124 ↗(On Diff #276367)

Good to know. Should I remove the check completely?

limo1996 marked an inline comment as done.Jul 8 2020, 6:03 AM
limo1996 added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
529

Yeah that's kind of the point of this revision.. I know we should not but I wanted to ask how can I solve it bcos I don't know how to add to arguments instead of rewrite them inside GenericOp definition.. Any suggestion is rly welcomed :)

ftynse added inline comments.Jul 8 2020, 6:04 AM
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
522 ↗(On Diff #276367)

Just change the return type and return llvm::None instead of -1.

mlir/test/Dialect/Linalg/invalid.mlir
124 ↗(On Diff #276367)

Yes

limo1996 updated this revision to Diff 276404.Jul 8 2020, 6:27 AM

getSymbolSource: llvm::Optional instead of -1

limo1996 marked 2 inline comments as done.Jul 8 2020, 6:28 AM
limo1996 marked an inline comment as done.Jul 8 2020, 7:39 AM
limo1996 added inline comments.
mlir/lib/Dialect/Linalg/Transforms/DropUnitDims.cpp
321 ↗(On Diff #276367)

So the problem is that custom builder requires types like int64_t etc. but autogenerated one requires corresponding attributes.. So I would suggest one more custom builder but with attributes. What do you think?

limo1996 updated this revision to Diff 276433.Jul 8 2020, 7:41 AM

Test cases for now autogenerated error messages removed

limo1996 marked 2 inline comments as done.Jul 8 2020, 7:44 AM
limo1996 marked 2 inline comments as done.Jul 9 2020, 9:09 AM
limo1996 added inline comments.
mlir/lib/Dialect/Linalg/Transforms/DropUnitDims.cpp
321 ↗(On Diff #276367)

Submited it as a bug. You can find it here: https://bugs.llvm.org/show_bug.cgi?id=46658

limo1996 marked an inline comment as done.Jul 10 2020, 5:08 AM
limo1996 updated this revision to Diff 277003.Jul 10 2020, 5:42 AM

changes evolved from revision D83158

limo1996 abandoned this revision.Jul 14 2020, 1:57 AM

Changes merged to D83158