This revision drops init_tensor arguments from Linalg on tensors and instead uniformizes the output buffers and output tensors to be consistent.
This significantly simplifies the usage of Linalg on tensors and is a stepping stone for
its evolution towards a mixed tensor and shape abstraction discussed in https://llvm.discourse.group/t/linalg-and-shapes/2421/19.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/docs/Dialects/Linalg.md | ||
---|---|---|
24 | couldn't parse this | |
25–31 | Something went wrong with formatting here. | |
55 | I would have provided at least a summary of the discussion here. | |
61 | ||
67 | "tensor" and "write-only" don't belong in the same phrase IMO. Did you mean that there is no data that is expected to be read from the tensor? | |
205–231 | Something wrong happened to the formatting here. Generally, the overall reformat of the document makes it hard to understand what changed. | |
356–360 | bad formatting | |
376–380 | bad formatting | |
399–413 | Even more bad formatting | |
428–438 | and more | |
507 | we have this in langref now |
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td | ||
---|---|---|
115 | Nit: getOperands().slice(0,1)? | |
325–327 | The comment mentions nWin but the check is for nPar. | |
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td | ||
148–150 | Can we than declare inputs and outputs as interface methods without default implementation? I expect the compiler/linker to complain if they are missing. | |
298 | ||
298 | ||
323–327 | Nit: isa+cast is an anti-pattern. | |
393 | ||
554–556 | I don't see how checking for the number of regions safeguards against manually-defined ops. Do they have no regions? Also, please add a message into assert. |
This looks mostly mechanical, LGTM after the comments are addressed.
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp | ||
---|---|---|
399–400 | This will not catch the case where the ops has N output _buffers_ and N results. Is this expected? | |
431 | This prints the entire type rather than just rank | |
446–447 | The check above (op->getNumResults() > linalgOp.getNumOutputs()) (with my comment) looks redundant to this. | |
507–508 | Nit: I think we use "region arguments" to refer to the arguments of the entry block of the region in LangRef, it's simpler than "region basic block arguments". | |
mlir/lib/Dialect/Linalg/Transforms/Bufferize.cpp | ||
32 | Spurious semicolon | |
mlir/lib/Dialect/Linalg/Transforms/DropUnitDims.cpp | ||
348 | Spurious semicolon | |
mlir/lib/Dialect/Linalg/Transforms/ElementwiseToLinalg.cpp | ||
28–38 | I'm not convinced this always does the right thing, could you add an explanation as to why it is sufficient to always take the first value with matching type or the first operand? | |
mlir/lib/Dialect/Linalg/Transforms/FusionOnTensors.cpp | ||
621 | It looks weird to call to_vector<4> only to assign the result to SmallVector<, 1> | |
mlir/test/Dialect/Linalg/invalid.mlir | ||
308–315 | Could you elaborate what is the current order and the order that would be required for this to work? The same information (or a reference here) from the assertion is also helpful to debug the cases where it triggers. |
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td | ||
---|---|---|
323–327 | Surprisingly annoying to swap map_range and make_filter_range. |
LGTM, with a minor suggestion for unnamed output parameter
mlir/docs/Dialects/Linalg.md | ||
---|---|---|
57 | For the shape-only case, would it make sense to really just define the shape instead, and not rely on a "dummy" named output argument? | |
60 | nit: built-in | |
mlir/lib/Dialect/Linalg/Transforms/Sparsification.cpp | ||
470 | Thanks for updating this file too! You may have a few merge conflicts due to my latest check in but they should be minor. | |
543 | I suspect this can be simplified now, but I can do that as a follow up | |
mlir/test/Dialect/Linalg/sparse_1d.mlir | ||
36 | e.g. here, arga is really not the output, but it just happens to define the right shape? |
mlir/test/Dialect/Linalg/sparse_1d.mlir | ||
---|---|---|
37 | here and below, should %s be %argb? or, otherwise, what does it mean to have an unused scalar incoming argument, and another one that is defined outside the kernel? I see it is also done for some other kernels without scalar invariants? |
couldn't parse this