This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Revisit the Linalg on tensors abstraction
ClosedPublic

Authored by nicolasvasilache on Dec 17 2020, 10:22 AM.

Details

Summary

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.

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Dec 17 2020, 10:22 AM

Finish the impl.

Drop debug spew.

nicolasvasilache edited the summary of this revision. (Show Details)Dec 18 2020, 3:24 AM
Harbormaster completed remote builds in B82933: Diff 312743.
ftynse added inline comments.Dec 18 2020, 5:22 AM
mlir/docs/Dialects/Linalg.md
24

couldn't parse this

25–31

Something went wrong with formatting here.

45

I would have provided at least a summary of the discussion here.

51
57

"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?

202–220

Something wrong happened to the formatting here. Generally, the overall reformat of the document makes it hard to understand what changed.

297–301

bad formatting

317–321

bad formatting

343–357

Even more bad formatting

379–392

and more

461

we have this in langref now

ftynse added inline comments.Dec 18 2020, 5:39 AM
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.

ftynse accepted this revision.Dec 18 2020, 8:22 AM

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
33

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
582

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.

This revision is now accepted and ready to land.Dec 18 2020, 8:22 AM
nicolasvasilache marked 10 inline comments as done.

Rebase on a better Linalg.md

nicolasvasilache marked an inline comment as done.Dec 18 2020, 8:25 AM
nicolasvasilache added inline comments.
mlir/docs/Dialects/Linalg.md
45

it's actually described below in init tensor / shape-only discussion and redundant.
Removed it from here.

202–220

yeah really sorry about that .. I landed a reformat RFC and rebased on top of it, should be better now.

nicolasvasilache marked 17 inline comments as done.Dec 18 2020, 9:28 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td
323–327

Surprisingly annoying to swap map_range and make_filter_range.
Leaving as 2021's problem.

nicolasvasilache marked an inline comment as done.

Address.

pifon2a accepted this revision.Dec 18 2020, 11:40 AM

Thank you, Nicolas!

aartbik accepted this revision.Dec 21 2020, 11:00 AM

LGTM, with a minor suggestion for unnamed output parameter

mlir/docs/Dialects/Linalg.md
47

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?

50

nit: built-in

mlir/lib/Dialect/Linalg/Transforms/Sparsification.cpp
413

Thanks for updating this file too! You may have a few merge conflicts due to my latest check in but they should be minor.

486

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?

aartbik added inline comments.Jan 5 2021, 11:01 AM
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?

mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td