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
3159

typo: mixed

3212

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?

3221

typo: be

3222

s/and be/and /

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

"||"?

2716

"||"?

3358–3359

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

3360

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

3380–3385

remove ? or is it "under construction"?

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

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
3212

rephrased things here and everywhere, thanks for noting!

3222

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.

3360

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
3201

insert -> inserts

3202–3203

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

3208

has the encodes -> encodes

3223

encodes that the -> specifies that the

3226

Doc description is missing an example.