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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td | ||
---|---|---|
211 | Nit: Please terminate with period. | |
262 | Missing doc. | |
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp | ||
311 | These are all missing doc comments - they do need at least a couple of lines however obvious it might appear to the authors now. | |
324–329 | I'm missing something here. Is this diff correctly generated? Where did the existing emitScalarImplementation of linalg.conv go? |
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 | ||
---|---|---|
248 | Don't iterator types (at least, their number) also depend on rank? | |
258 | 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. | |
259 | @nicolasvasilache should this use "window" type for appropriate loops? | |
264 | This function is long enough to deserve having its definition in a .cc file. | |
290 | typeof is a non-standard GCC extension, this will break most other compilers. Why do you even need explicit template instantiation here? | |
313 | 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 | ||
324–329 | I see it at line 390.... | |
343 | Can this at least use a consistent naming scheme: m1,m2,m3? |
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp | ||
---|---|---|
311 | I think @bondhugula meant function-level comments. | |
315 | All comments must use proper English grammar, with capitalization and periods. | |
328 | Nit: this comment would make more sense above the first line. | |
mlir/test/Dialect/Linalg/loops.mlir | ||
1301–1309 | 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. |
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 | ||
---|---|---|
324–329 | If that's still there, how is Conv2D different from that? Isn't linalg.conv the Conv2D here? |
Most of the comments addressed
mlir/test/Dialect/Linalg/loops.mlir | ||
---|---|---|
1301–1309 | Here I assume that input array has the padding and the output one not.. Should I do it in another way? |
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp | ||
---|---|---|
324–329 | 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 |
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 | ||
324–329 | 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. | |
1301–1309 | 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. |
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) | |
1301–1309 | Ok. It's mentioned in ODS documentation. We can add std.assert to our TODO list. |
Loop type of kernel iterators changed from parallel to reduction as those cannot be parallelized.
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 | ||
---|---|---|
252 | This sounds overly restrictive in two ways:
| |
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? |
mlir/test/Dialect/Linalg/loops.mlir | ||
---|---|---|
18 | You are right. Sorry |
mlir/lib/Dialect/Linalg/Transforms/Loops.cpp | ||
---|---|---|
324–329 |
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.
Yes, but I was asking what would happen to the current linalg.conv which is equivalent to this conv2d. |
Nit: Please terminate with period.