This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Conv1D, Conv2D and Conv3D added as named ops
ClosedPublic

Authored by limo1996 on Jul 15 2020, 8:08 AM.

Details

Summary

This commit is part of a greater project which aims to add
full end-to-end support for convolutions inside mlir. The
reason behind having conv ops for each rank rather than
having one generic ConvOp is to enable better optimizations
for every N-D case which reflects memory layout of input/kernel
buffers better and simplifies code as well. We expect plain linalg.conv
to be progressively retired.

Diff Detail

Event Timeline

limo1996 created this revision.Jul 15 2020, 8:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2020, 8:08 AM
bondhugula requested changes to this revision.Jul 15 2020, 8:28 PM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
186

Nit: Please terminate with period.

237

Missing doc.

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

These are all missing doc comments - they do need at least a couple of lines however obvious it might appear to the authors now.

368–373

I'm missing something here. Is this diff correctly generated? Where did the existing emitScalarImplementation of linalg.conv go?

This revision now requires changes to proceed.Jul 15 2020, 8:28 PM
ftynse requested changes to this revision.Jul 16 2020, 4:22 AM

This is a reasonable first take. Please make sure to document your assumptions, e.g. the order of dimensions. Also make sure the long documentation of the "base" class gets visible when you generate documentation from tablegen.

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

Don't iterator types (at least, their number) also depend on rank?

233

The "input rank * 2" as number of loops sounds fragile to me. Please add documentation about this assumption somewhere, it will have to be revisited when we want to model batch convolutions.

234

@nicolasvasilache should this use "window" type for appropriate loops?

239

This function is long enough to deserve having its definition in a .cc file.

265

typeof is a non-standard GCC extension, this will break most other compilers. Why do you even need explicit template instantiation here?

288

This does not correspond to the maps the code above constructs. The code will produce (d0,d1,d2,d3) -> (d0 + d2, d1 + d3).

mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
368–373

I see it at line 390....

387

Can this at least use a consistent naming scheme: m1,m2,m3?

limo1996 updated this revision to Diff 278520.Jul 16 2020, 9:33 AM
limo1996 marked 10 inline comments as done.

10/12 comments resolved

ftynse added inline comments.Jul 16 2020, 9:59 AM
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
355

I think @bondhugula meant function-level comments.

359

All comments must use proper English grammar, with capitalization and periods.

372

Nit: this comment would make more sense above the first line.

mlir/test/Dialect/Linalg/loops.mlir
1303–1311

This currently generates out-of-bounds accesses. Please also update the loop bound generator to either (a) handle the access expressions properly or (b) return an error in presence of such expressions. (b) is simpler for this commit, but we will ultimately want (a). Your choice.

bondhugula requested changes to this revision.Jul 16 2020, 12:16 PM

Could you please rewrite the commit summary? The current one isn't really one - that's more of a plan.

mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
368–373

If that's still there, how is Conv2D different from that? Isn't linalg.conv the Conv2D here?

This revision now requires changes to proceed.Jul 16 2020, 12:16 PM
limo1996 updated this revision to Diff 279458.Jul 21 2020, 2:33 AM
limo1996 marked 5 inline comments as done.

Most of the comments addressed

mlir/test/Dialect/Linalg/loops.mlir
1303–1311

Here I assume that input array has the padding and the output one not.. Should I do it in another way?

limo1996 marked 2 inline comments as done.Jul 21 2020, 2:49 AM
limo1996 added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
368–373

There is a new approach to convolution where we will have named ops for each rank so we can optimize better for each case. For more information you can reach out to @nicolasvasilache

ftynse added inline comments.Jul 21 2020, 3:33 AM
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
1129

I think this is sufficiently clear to avoid additional comments in the argument list. If you want to keep a comment above the expression, please use proper English grammar with capitalization and punctuation. I already mentioned this guideline in this diff, it applies to _all_ comments.

mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
368–373

We expect plain linalg.conv to be progressively retired. It was mimicking TF convolution, which isn't necessarily generic or transformation-friendly enough. This will happen after we reach feature parity.

mlir/test/Dialect/Linalg/loops.mlir
18

Please avoid spurious whitespace changes.

1303–1311

This assumption should be explicit in the op documentation (in ODS).

Ideally, we should generate std.assert on sizes now that we have that operation, but it can be done consistently for all of Linalg, in a separate commit.

limo1996 updated this revision to Diff 279508.Jul 21 2020, 6:45 AM
limo1996 marked 7 inline comments as done.

Documentation of ConvOpBase now included comment that input buffers need to be padded
+ other minor changes

mlir/test/Dialect/Linalg/loops.mlir
18

It is not spurious. It is intended as I think an empty line after 5 lines of code increases readability. Actually I am just following the pattern set above (ever sixth line is empty)

1303–1311

Ok. It's mentioned in ODS documentation. We can add std.assert to our TODO list.

limo1996 updated this revision to Diff 279524.Jul 21 2020, 8:05 AM

Loop type of kernel iterators changed from parallel to reduction as those cannot be parallelized.

ftynse requested changes to this revision.Jul 22 2020, 4:53 AM

Please update the diff/commit description as requested before. It should explain why you are doing the change, https://mlir.llvm.org/getting_started/Contributing/#commit-messages.

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

This sounds overly restrictive in two ways:

  • there will be bad accesses _unless_ the size of the input is greater than or equal to the size of the output + size of the kernel - 1, not just when the size of the input is equal to the size of the output;
  • the behavior is explained in terms of a specific code generation scheme; it should rather say something like "the behavior of the op is undefined if ...".
mlir/test/Dialect/Linalg/loops.mlir
18

It is. It isn't anyhow related to what this diff does, that is "Conv1D, Conv2D and Conv3D added as named ops". Piggy-backing on commits to do irrelevant changes is bad practice that makes it harder to track changes in the future (irrelevant commits show up in blame). If you want to make a non-functional cleanup change, submit a separate diff with a proper description and put "NFC" in the name to simplify the review.

Speaking about readability, the empty lines between the blocks seem to be attributable to https://github.com/llvm/llvm-project/commit/e36337a998a6be39d65872eab3e3e2291b6518b9, where there was a different number of lines per block. So I doubt the claim that every sixth line is intentionally empty. Even if it had been the case, I would have challenged a rule based on syntax rather than semantics. It makes more sense to separate blocks by semantics (maps with dilation / maps without dilation) than by the number of lines. Your code below has a snippet of 28 CHECK lines without vertical whitespace, why haven't those been splitted into 6 different blocks then?

This revision now requires changes to proceed.Jul 22 2020, 4:53 AM
limo1996 updated this revision to Diff 279825.Jul 22 2020, 7:35 AM

evolve & merge

limo1996 marked 3 inline comments as done.Jul 22 2020, 7:36 AM
limo1996 added inline comments.
mlir/test/Dialect/Linalg/loops.mlir
18

You are right. Sorry

limo1996 updated this revision to Diff 279827.Jul 22 2020, 7:43 AM
limo1996 marked 2 inline comments as done.

comment regarding input sizes done

bondhugula added inline comments.Jul 22 2020, 11:38 AM
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
368–373

We expect plain linalg.conv to be progressively retired. It was mimicking TF

Please add a comment on this. Ideally, this should be mentioned in the commit summary or else it wouldn't clear to those outside of your internal discussion.

There is a new approach to convolution where we will have named ops for each rank
so we can optimize better for each case. For more information you can reach out to

Yes, but I was asking what would happen to the current linalg.conv which is equivalent to this conv2d.

bondhugula resigned from this revision.Jul 22 2020, 11:38 AM
limo1996 updated this revision to Diff 280348.Jul 24 2020, 1:00 AM

Small refactoring

limo1996 edited the summary of this revision. (Show Details)Jul 24 2020, 1:01 AM
limo1996 marked an inline comment as done.
limo1996 updated this revision to Diff 280939.Jul 27 2020, 9:04 AM

Computation of indexes split into multiple to make windows tests (hopefully) pass

ftynse accepted this revision.Jul 28 2020, 5:45 AM
This revision is now accepted and ready to land.Jul 28 2020, 5:45 AM

Could you please rebase?

Done

This revision was landed with ongoing or failed builds.Jul 29 2020, 7:40 AM
This revision was automatically updated to reflect the committed changes.