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.

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

ftynse added inline comments.Dec 18 2020, 5:39 AM
mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
115

Nit: getOperands().slice(0,1)?

312–314

The comment mentions nWin but the check is for nPar.

mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td
148–160

Can we than declare inputs and outputs as interface methods without default implementation? I expect the compiler/linker to complain if they are missing.

318
318
343–347

Nit: isa+cast is an anti-pattern.

413
574–576

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

447–448

The check above (op->getNumResults() > linalgOp.getNumOutputs()) (with my comment) looks redundant to this.

508–509

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
26

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–318

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
55

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

205–231

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
343–347

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

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