Page MenuHomePhabricator

[mlir] Add subtensor_insert operation
ClosedPublic

Authored by nicolasvasilache on Oct 1 2020, 7:33 AM.

Details

Summary

This revision adds the subtensor_insert operation.
This operation is discussed in https://llvm.discourse.group/t/an-update-on-linalg-on-tensors/1878
and is useful for extending linalg + transformations on tensors.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2020, 7:33 AM
nicolasvasilache requested review of this revision.Oct 1 2020, 7:33 AM
pifon2a accepted this revision.Oct 1 2020, 10:31 AM
pifon2a added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
3105

typo: mixed

3158

shouldn't it be always tensor-rank number of static offsets with all -1s in case we have none and rank - |static offsets| number of dynamic offsets?

3167

typo: be

3168

s/and be/and /

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2687

"||"?

2716

"||"?

3253–3254

Would this also work SmallVector<int64_t, 4> staticSizesVector(rank, ShapedType::kDynamicSize); ?

3255

Here staticStridesVector == staticOffsetsVector, can we have just one vector?

3275–3280

remove ? or is it "under construction"?

mlir/test/IR/core-ops.mlir
900

please, provide the test for the mixed case.

This revision is now accepted and ready to land.Oct 1 2020, 10:31 AM
nicolasvasilache marked 10 inline comments as done.Oct 2 2020, 3:29 AM
nicolasvasilache added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
3158

rephrased things here and everywhere, thanks for noting!

3168

rephrased a bit differently

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
2687

if parseExtraOperand is not specified we want to skip calling it.

2716

false is the "success" here so if parseExtraOperand is not specified we do not want an extra resolveOperand.

3255

No, they don't have the same sentinel.
The dynamic size sentinel was historically encoded to -1 and evolving that has been proven very intrusive ..

nicolasvasilache marked 5 inline comments as done.Oct 2 2020, 3:29 AM

Address review comments.

This revision was landed with ongoing or failed builds.Oct 2 2020, 3:34 AM
This revision was automatically updated to reflect the committed changes.

This seems to have broken GCC 5 (https://buildkite.com/mlir/mlir-core/builds/8301#e6b6e5a7-85bd-4454-ae2b-da3b6cf84b8a). Could you update? Or should we revert?

had a potential fix in flight but did not land, trying again

bondhugula added inline comments.Oct 3 2020, 1:41 PM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
3147

insert -> inserts

3148–3149

This description would make it look like the op should be called a tensor_insert. Consider rephrasing along the lines: `... while specifying offsets, ..., and sizes for the result tensor".

3154

has the encodes -> encodes

3169

encodes that the -> specifies that the

3172

Doc description is missing an example.