This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Support complex numbers in Linalg promotion
ClosedPublic

Authored by tpopp on Apr 6 2021, 3:41 AM.

Details

Summary

FillOp allows complex ops, and filling a properly sized buffer with
a default zero complex number is implemented.

Diff Detail

Event Timeline

tpopp created this revision.Apr 6 2021, 3:41 AM
tpopp requested review of this revision.Apr 6 2021, 3:41 AM
nicolasvasilache added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp
63

should we start being more deliberate about using the data layout here and in other places ?
@ftynse

ftynse added inline comments.Apr 7 2021, 4:32 AM
mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp
63

Yes, it would be more future-proof to query the size in bytes from the data layout, even if it's for the element type for now (there is no support for complex type in the data layout, yet). I would also suggest to construct the DataLayout object at the pass entry and forward it down here to avoid recomputation.

285

Nit: just cast to catch cases of other element types?

288

Nit: Please make braces symmetric

tpopp updated this revision to Diff 336768.Apr 12 2021, 1:08 AM
tpopp marked 3 inline comments as done.

Switch to using DataLayout.

tpopp marked an inline comment as done and an inline comment as not done.Apr 12 2021, 1:09 AM
tpopp added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp
63

I switched to using DataLayout, but to cache the computation will require changing AllocBufferCallbackFn (https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h#L213). I don't know if that would be a good change?

The reason that change would be needed is because the alternative is to only use this in the default callback, but that can be constructed and used across multiple modules which leads to segfaults from using dangling pointers.

tpopp marked an inline comment as done.Apr 12 2021, 1:30 AM
tpopp added inline comments.
mlir/lib/Interfaces/DataLayoutInterfaces.cpp
55–58 ↗(On Diff #336768)

Actually implemented here https://reviews.llvm.org/D100289

ftynse added inline comments.Apr 13 2021, 9:40 AM
mlir/lib/Interfaces/DataLayoutInterfaces.cpp
55–58 ↗(On Diff #336768)

You can add that diff as parent for this to create a "stack" of diffs only showing relevant changes. Click "edit related revisions" on the top right, and then "edit parent revisions"

tpopp updated this revision to Diff 337372.Apr 14 2021, 1:44 AM

Rebase onto parent commit

ftynse added inline comments.Apr 16 2021, 9:04 AM
mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp
56

Use something like DataLayout::closest(size.getDefiningOp()), otherwise it's the default layout that ignores the specification. Incidentally, that is also why you may want to pass it in from above.

63

I expect other things will eventually want data layout as well, so it's okay to change the callback signature.

nicolasvasilache added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp
63

+1 re. ok to change signature.
Note however that this will break a bunch of things in IREE.

@ftynse is there a future in which the DataLayout could be auto-whatever from a builder insertion point (haven't thought this through, just basic idea that passed through my mind) ?

Also @maheshkhanwalkar for visibility if signature changes.

ftynse added inline comments.Apr 19 2021, 5:49 AM
mlir/lib/Dialect/Linalg/Transforms/Promotion.cpp
63

You can already do DataLayout::closest(builder.getInsertionPoint()). Doing it the other way around feels like a layering violation, builder isn't supposed to know about individual interface.

tpopp updated this revision to Diff 338797.Apr 20 2021, 3:26 AM
tpopp marked 3 inline comments as done.

Forward DataLayout object to buffer allocation.

tpopp added a comment.Apr 20 2021, 3:27 AM

I forwarded the DataLayout through several layers of functions. I started at the last point that the call sequence has the originating LinalgOp and passed it through as an additional argument rather than inside of any of the LinalgPromotionOption data structures. Please let me know what you think.

tpopp marked an inline comment as done.Apr 23 2021, 4:57 AM

Friendly ping.

ftynse accepted this revision.Apr 26 2021, 12:43 AM

Sorry for the delay.

This revision is now accepted and ready to land.Apr 26 2021, 12:43 AM