This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Move some bufferization patterns to std
AbandonedPublic

Authored by silvas on Oct 21 2020, 4:49 PM.

Details

Summary

ConstantOp and TensorCastOp are part of std, so it makes sense for their
bufferizations to live there.

Also, add a couple TODO's for cases that don't work yet: rank-0 tensor
constants and tensor constants with an extent that is 0.

Diff Detail

Event Timeline

silvas created this revision.Oct 21 2020, 4:49 PM
silvas requested review of this revision.Oct 21 2020, 4:49 PM
silvas updated this revision to Diff 299833.Oct 21 2020, 5:44 PM

fix BUILD_SHARED_LIBS

herhut requested changes to this revision.Oct 22 2020, 4:26 AM
herhut added inline comments.
mlir/lib/Dialect/StandardOps/Transforms/Bufferize.cpp
42

Wouldn't the most straight-forward way be to just allocate a size-0 memref?

56

Use getNumElements() instead.

62

So this entire vector dance is only required because store cannot write tensor values? That seems like a short cut to me rather than the best way to implement this. Why not use a tensor constant followed by a tensor_store? That seems more canonical to me and also should handle 0-element tensors correctly.

80

Can we at least avoid the dependency on linalg here and instead use a memref_reshape here?

This revision now requires changes to proceed.Oct 22 2020, 4:26 AM

Note that this is just moving code with some light touch-up. We don't need to block on architectural discussion for the pattern itself.

mlir/lib/Dialect/StandardOps/Transforms/Bufferize.cpp
62

Why not use a tensor constant followed by a tensor_store?

This is the pattern that lowers tensor constants :)

I agree in general though, that this pattern is likely in need some some rethinking (possibly based on Rahul's work on std.global_memref)

silvas marked an inline comment as done.Oct 22 2020, 11:56 AM
silvas added inline comments.
mlir/lib/Dialect/StandardOps/Transforms/Bufferize.cpp
42

Good idea. This patch is just a mechanical move with a light touch-op, so I added that in a comment.

80

That op doesn't seem to exist upstream. Maybe you folks could upstream the one from mhlo?

silvas updated this revision to Diff 300064.Oct 22 2020, 11:57 AM

address comments

herhut added inline comments.
mlir/lib/Dialect/StandardOps/Transforms/Bufferize.cpp
80

That is in the work and I believe the op is landing as I write this. I am ok with moving this over to std even though it is using a vector but I'd rather avoid the dependency on linalg. Linalg depending on std seems natural but std depending on linalg seems wrong.

The reshape and its lowering should be available soon (FYI @pifon2a).

silvas abandoned this revision.Nov 2 2020, 5:43 PM

Abandoning. This transition is already moving forward in a different way.