This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Lower padding attribute for pooling ops
ClosedPublic

Authored by alberto-magni on Oct 6 2020, 10:34 AM.

Details

Summary

Update linalg-to-loops lowering for pooling operations to perform
padding of the input when specified by the corresponding attribute.

Diff Detail

Event Timeline

alberto-magni created this revision.Oct 6 2020, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2020, 10:34 AM
alberto-magni requested review of this revision.Oct 6 2020, 10:34 AM

THanks for filling this gap! Added @hanchung to reviewers list who knows the details a bit more.

FYI, this was added to linalg, but after using padding attributes in IREE, the fact that pooling op/conv ops are not tilable if they are padded made it untenable for our use case (in IREE). So AFAIK, IREE doesnt use the padding attributes. This change is still valid, but I think we want to deprecate the use of padding attributes in the future (@nicolasvasilache for comments). If you are relying on this would be good to know if it is possible to not use the padding attributes.

mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
391

I am not clear if this is actually the case. Its better to get Hanhan take a look at this. (added him as a reviewer)

mravishankar requested changes to this revision.Oct 6 2020, 4:43 PM
This revision now requires changes to proceed.Oct 6 2020, 4:43 PM

Hi Mahesh :)
I am experimenting with lowering onnx to linalg.
ONNX ops like conv and pooling have padding attributes very much like linalg does right now. So linalg attributes would be helpful.
https://github.com/onnx/onnx/blob/master/docs/Operators.md#attributes-41
It is also possible to do without the onnx padding attributes, so the deprecation of the padding attributes would not be a show-stopper for me.

Thanks for adding this! I left two concerns below. I stoped adding this because we could drop padding attributes in the future as @mravishankar mentioned. And there are cases that I don't have answers yet, we can iterate on one of the approach for padding value and revisit padding dimensions later.

In IREE, we have a pass to extract padding attributes and create a pad op, so we can fully parallelize the computation on conv ops and pooling ops, fyi.

mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
390

I think we have an issue for padding values. Ideally, the padding value for pooling ops are different, eg, we probably want minimal int/float value for max_pooling, zero for sum_pooling, maximal int/float value for min_pooling.

Another way is to have an initial value, and we can use initial value as padding value.

If we don't have an initial value, we need different padding values for different pooling ops.

391

Yes, this is correct because we don't carry any batch/channel dimension to Linalg.

I think we're considering to carry that information to Linalg but never get a conclusion.

The conv op does follow the tf semantics to have batch/window dims, but the pooling ops currently dones't. The reason is that I was starting from HLO ops and they don't have such semantics. We can consider to add them, but for now it's not the case.

https://www.tensorflow.org/api_docs/python/tf/nn/pool

alberto-magni added inline comments.Oct 7 2020, 10:24 AM
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
390

Ah right, of course makes sense.
This would require to change the line above:

Value zero = std_constant(type, b.getZeroAttr(type));

to use a parameter instead.
I can make the change, if you think it is worth.

hanchung added inline comments.Oct 8 2020, 2:49 AM
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
390

This makes sense to me without changing the op semantics. Let do it as this way to make progress. thanks!

alberto-magni added inline comments.Oct 8 2020, 3:00 AM
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
390

Ok, perfect. I'll follow up on this tomorrow.

Specialize padding value depending on the semantics of the operation.
Add testing for integer pooling.

hanchung accepted this revision.Oct 12 2020, 9:13 AM

Just few nits, thanks for adding this!

mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
274–277

Remove the blank line between the function and comments.

Use /// instead of //

289

/*Negative=*/true

366

I would name it as padValue because zero is the context inside getPadValueAttr<ConvOp>.

Address comments

Thank you!

Fix bogus commit.

Commend addressed.
Please go head and merge it when you like.

Thank you!

This revision was not accepted when it landed; it landed in state Needs Review.Oct 13 2020, 2:12 PM
This revision was automatically updated to reflect the committed changes.