This revision allows representing a reduction at the level of linalg on tensors for generic ops by uniformizing with the named ops approach.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
80 ms | linux > MLIR.EDSC::builder-api-test.cpp | |
4,340 ms | windows > MLIR.EDSC::builder-api-test.cpp |
Event Timeline
mlir/include/mlir/Dialect/Linalg/EDSC/Builders.h | ||
---|---|---|
48 | The comment looks outdated after the changes to the function signature. | |
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td | ||
524 | {other-attributes}? (Generally, I see the notation in this file is inconsistent between [other-attributes] and {other-attributes}, so maybe this change is intentional?). | |
614 | Is this paragraph removed because this is no longer the case? | |
621 | "update" instead of "updates"? | |
639 | What do you think about the syntax which keeps type annotations at the end (while still introducing ins and friends)? | |
718 | Missing curly brace? | |
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp | ||
46 | In the function above, this parameter is called outputBufferTypes without the "s" in "Buffers". Also see initTensorTypes below. | |
mlir/test/Dialect/Linalg/invalid.mlir | ||
394 | Why is this no longer a test? | |
527 | Likewise. |
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td | ||
---|---|---|
614 | correct, the SSA values don't escape control flow is not true anymore (e.g. scf.for with yield that I am going to take advantage of in the future). | |
639 | It has 2 drawbacks that I can see right now:
| |
mlir/test/Dialect/Linalg/invalid.mlir | ||
394 | it was duplicated, see the next test. | |
527 | this is no longer valid, there is no such thing as #outputs anymore (previously independently specified by the args_out attribute). |
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td | ||
---|---|---|
639 |
|
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td | ||
---|---|---|
639 |
Yes that is the unsugared form, it quickly becomes hard to read when you have enough operands.
Because parameter packs based on operand_segment_sizes need to be sugared otherwise they are hard to segment out.
This relates to @mravishankar's comment on https://reviews.llvm.org/D87767. TL;DR I agree that we want to go towards a func-like syntax where each arg is followed by its type. The situation is much better than it was before though: if we have 3 parameter packs with 2, 3 and 4 arguments there is strictly less jumping through hoops to determine the type of second argument of the third pack (you just need to look for the 2nd local type instead of looking for the 7^th global type).
We could also put the region before the type arguments, would that alleviate part of the problem ?
Consistency wins here, first I need to fix the semantics gap and uniformize with named ops (see https://reviews.llvm.org/D87767).
The status quo is now https://reviews.llvm.org/D87767, the generic ops have weaker semantics and need to be updated to allow tensors + reductions. Once the harder functional changes are landed, we can iterate on a better syntax. |
I like the new syntax and encoding! Great cleanup. Some more verification of valid forms would be nice.
mlir/include/mlir/Dialect/Linalg/EDSC/Builders.h | ||
---|---|---|
51 | I assume you mean resultTensorTypes here? | |
53 | Is this an alternative way to encode tensor results? | |
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td | ||
524 | This is not the actual syntax, right? There is an attrs there. | |
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp | ||
360 | I would have expected init_tensors not to count here. They only exist for reductions on tensors, so they are implied. But this verification is not very precise anyway. | |
1222 | Maybe assign to a local? | |
1450 | nit: parseColonTypeList | |
1507 | printArrowTypeList or printOptionalArrowTypeList? | |
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp | ||
446 | Left over? | |
512–521 | I know this is a carry over but why UnknownLoc? | |
517 | Reformat. | |
mlir/test/lib/Transforms/TestBufferPlacement.cpp | ||
42 | Not sure why this exists here. Can this not use the pattern from TensorToBuffers.h or is that not exposed there? |
mlir/include/mlir/Dialect/Linalg/EDSC/Builders.h | ||
---|---|---|
53 | Just stale stuff, cleaned up, thanks! | |
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td | ||
524 | Indeed thanks! Ideally this would go away but this CL has grown fat and I am running out of patience writing throwaway parser/printer code. | |
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp | ||
360 | Ack, more work is needed to make reductions + tensors really work with Linalg. | |
1222 | Not getting this, could you please elaborate? | |
mlir/test/lib/Transforms/TestBufferPlacement.cpp | ||
42 | It's not exposed and it is not exposable without deeper refactorings because we don't want TensorToBuffer.h to depend on Linalg. |
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp | ||
---|---|---|
1222 | It was just a nit to do auto iteratorTypes = cast<LinalgOp>(op).iterator_types() and then use that here and below. | |
mlir/test/lib/Transforms/TestBufferPlacement.cpp | ||
42 | There already is a populateConvertLinalgOnTensorsToBuffersPattern function, it is just not exposed. It should be enough to just expose that function and call it here. No need to do this in this change, I can clean that up, too, once this landed. We need to figure out where to put all the tensor to buffers pattern anyway, as different dialects will need them and having a populate function in passes/transforms/rewrites seems the right approach to me. @tpopp FYI as you looked into patterns for shape.assuming. |
The comment looks outdated after the changes to the function signature.