Page MenuHomePhabricator

[mlir][linalg] Extend tiled_loop to SCF conversion to generate scf.parallel.

Authored by pifon2a on Sep 3 2021, 5:39 AM.

Diff Detail

Event Timeline

pifon2a created this revision.Sep 3 2021, 5:39 AM
pifon2a requested review of this revision.Sep 3 2021, 5:39 AM
pifon2a updated this revision to Diff 370560.Sep 3 2021, 5:41 AM

remove new line

pifon2a updated this revision to Diff 370588.Sep 3 2021, 8:12 AM

Update variable names.

tpopp added inline comments.Sep 3 2021, 8:25 AM

At this point, the op should be limited to being entirely bufferized or not at all bufferized. Can you add this check to the verify method also? It could be as a followup as there's no very method currently.

I say this because partial bufferization along with this method would be very confusing and could easily lead to incorrect assumptions/bugs


clang tidy checks


This system relies on the assumption that a tiled loop with iteration patterns [parallel, sequential, parallel, sequential] can be reordered to [parallel, parallel, sequential, sequential]. Is that true and specified?


Maybe use builder here. This is relying on the caller of this never creating a new OpBuilfer from the rewriter passed to it (relying on two objects actually being the same). That could be easy to break and annoying to debug.


I don't think this is enough. If one op uses the result of another op in the body, I think it will still point to the original op rather than the new one.

%0 = add %a, %b
%1 = negate %0

Will result in
%new0 = add %a, %b
%new1 = negate %0

Won't it?

pifon2a updated this revision to Diff 370593.Sep 3 2021, 8:44 AM
pifon2a marked 5 inline comments as done.

Address the comments.

pifon2a added inline comments.Sep 3 2021, 8:47 AM

Unfortunately, comprehensive bufferization relies on tiled_loop with mixed buffers-tensors. It adds bufferized memref operands to the tiled_loop without removing the tensor ones and then canonicalization kicks in to clean this up. It does this because it has to be in-place.


I think it is true for the tiling that we support, since we are generating a perfectly nested loop where loop ctrl vars don't depend on each other. I will add a comment.


good catch, thanks.


builder.clone(op, bvm) also maps old results to new ones.

tpopp accepted this revision.Sep 3 2021, 8:52 AM
tpopp added inline comments.

What do you think about a name that specifies the "only" fact? hasNoTensorSemantics or hasOnlyBufferSemantics.

This revision is now accepted and ready to land.Sep 3 2021, 8:52 AM
This revision was landed with ongoing or failed builds.Sep 3 2021, 9:06 AM
This revision was automatically updated to reflect the committed changes.
pifon2a added inline comments.Sep 3 2021, 9:08 AM

hasBufferSemantics is what's used by mlir/Dialect/Linalg/IR/