This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Implement padding for linalg.conv and lowering to loops.
ClosedPublic

Authored by hanchung on Mar 5 2020, 8:09 PM.

Details

Summary

To enable this, two changes are needed:

  1. Add an optional attribute padding to linalg.conv.
  2. Compute if the indices accessing is out of bound in the loops. If so, use the

padding value 0. Otherwise, use the value derived from load.

In the patch, the padding only works for lowering without other transformations,
e.g., tiling, fusion, etc.

Diff Detail

Event Timeline

hanchung created this revision.Mar 5 2020, 8:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2020, 8:09 PM
hanchung retitled this revision from [mlir][Linalg] Implement padding for lowering linalg to loops. to [mlir][Linalg] Implement padding for linalg.conv and lowering to loops..Mar 5 2020, 8:20 PM
mravishankar requested changes to this revision.Mar 6 2020, 9:54 AM
mravishankar added inline comments.
mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp
180

Could you add some comments about whats happening here?

208

I am not sure we need to restrict this for the static shape case. You can always get the dimension dynamically and generate code for that. Just generate a dim operation?

This revision now requires changes to proceed.Mar 6 2020, 9:54 AM
hanchung updated this revision to Diff 248802.Mar 6 2020, 11:46 AM
hanchung marked 4 inline comments as done.

Address comments

mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp
180

Done.

208

Done.
Yes, you're right, we can always get the dimension here. Also change the tests to dynamic shape.

mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
279

This must be documented in great detail as it is very intrusive on a bunch of things.
You also want to go into all Linalg transformations and ensure that conv + dilations fails.
I will create a trait for that and refactor later.

mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
905

Please add a // TODO(ntv): add a level of indirection to linalg.generic.

mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp
187

Please add a // TODO(ntv): add a level of indirection to linalg.generic.

mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp
346

llvm_unreachable("Unexpected conv with padding");

here and everywhere in other transformations (tiling, fusion, promotion etc.)

431

nit: doesn't have

mlir/test/Dialect/Linalg/roundtrip.mlir
229

the semantics of padding aren't clear to me, please document them in the op definition.

nicolasvasilache requested changes to this revision.Mar 7 2020, 10:59 AM
This revision now requires changes to proceed.Mar 7 2020, 10:59 AM
herhut resigned from this revision.Mar 9 2020, 6:18 AM
herhut added a subscriber: herhut.
hanchung updated this revision to Diff 249503.Mar 10 2020, 2:51 PM
hanchung marked 6 inline comments as done.

Address comments.

hanchung marked an inline comment as done.Mar 10 2020, 2:52 PM
hanchung added inline comments.
mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp
187

Should this for another line, i.e., if (!convOp.padding())?

mravishankar resigned from this revision.Mar 11 2020, 12:02 PM

This looks fine to me now. But please wait for ntv to accept

mravishankar requested changes to this revision.Mar 12 2020, 12:40 AM
mravishankar added a subscriber: mravishankar.
mravishankar added inline comments.
mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp
190

you only need to iterate over the "window" dimensions. No need to check these conditions on the batch or channels dimensions

This revision now requires changes to proceed.Mar 12 2020, 12:40 AM
rriddle added inline comments.Mar 12 2020, 9:52 AM
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
325

nit: When using a DenseElementsAttr please do not use attributes as the value type whenever possible, it is very expensive(we have to create a new attribute, which requires acquiring a lock). If the value is an integer, use APInt. If the width of the integer is statically known, like in this case, use the corresponding C++ type. In this case you can use getValue<int64_t>.

hanchung updated this revision to Diff 250056.Mar 12 2020, 2:28 PM
hanchung marked 3 inline comments as done.

Address comments and clamp indices before accesing input.

stellaraccident requested changes to this revision.Mar 12 2020, 4:26 PM
stellaraccident added a subscriber: stellaraccident.
stellaraccident added inline comments.
mlir/test/Dialect/Linalg/loops.mlir
224

Typo: // CHECK-LABEL

This revision now requires changes to proceed.Mar 12 2020, 4:26 PM
hanchung updated this revision to Diff 250090.Mar 12 2020, 4:41 PM

Fix the test due to clamping indices accessing.

hanchung updated this revision to Diff 250091.Mar 12 2020, 4:42 PM

Fix typo in the check.

hanchung marked an inline comment as done.Mar 12 2020, 4:42 PM
mravishankar accepted this revision.Mar 12 2020, 6:13 PM
mravishankar added inline comments.
mlir/test/Dialect/Linalg/loops.mlir
243

Nit : Since you are generating the affine map in this change, better to check that you have the right affine map.

mehdi_amini added inline comments.Mar 12 2020, 9:16 PM
mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp
189

Do you need a vector here? Seems like you're only every using a single value (the last one) at a given time.

229

What is the guarantee that conds isn't empty here?

hanchung updated this revision to Diff 250131.Mar 12 2020, 10:56 PM
hanchung marked 4 inline comments as done.

Address comments.

  • Capture the affine_map for affine.max operation.
  • Change a bit logic for checking out-of-boundary access.
mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp
189

Yes, I need a vector here. The reason is that a ValueHandle can only be captured once.

229

In conv op, I think it will never happen. However, I updated the logic a bit and it looks simpler to me. Now it contains a true value in the beginning, so it's guarantee that conds isn't empty at all.

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