This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Fix SubTensorInsertOp semantics
ClosedPublic

Authored by nicolasvasilache on Jan 20 2021, 12:01 PM.

Details

Summary

Like SubView, SubTensor/SubTensorInsertOp are allowed to have rank-reducing/expanding semantics. In the case of SubTensorInsertOp , the rank of offsets/sizes/strides should be the rank of the destination tensor.

Also, add a builder flavor for SubTensorOp to return a rank-reduced tensor.

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Jan 20 2021, 12:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2021, 12:01 PM
ThomasRaoux accepted this revision.Jan 20 2021, 12:07 PM

LGTM!

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

nit: I think you can do SmallVector<int64_t, 4> staticOffsetsVector(rank, ShapedType::kDynamicStrideOrOffset);

This revision is now accepted and ready to land.Jan 20 2021, 12:07 PM
pifon2a accepted this revision.Jan 20 2021, 12:10 PM
pifon2a added inline comments.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
3470–3471

nit:
maybe initialize the vector in one line ?

SmallVector<int64_t, 4> staticOffsetsVector(rank, ShapedType::kDynamicStrideOrOffset);

here and everywhere.

3564

What if someone passes a type that cannot be cast to RankedTensorType? Maybe, do a dynamic cast and then assert?

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

nit: wrap the line

ThomasRaoux added inline comments.Jan 20 2021, 12:12 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
3564

cast<> already asserts so it is usually better than doing dyn_cast+assert

pifon2a accepted this revision.Jan 20 2021, 12:16 PM
pifon2a added inline comments.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
3564

I thought that cast<> asserts but does not provide a meaningful error message.

This revision was landed with ongoing or failed builds.Jan 20 2021, 12:17 PM
This revision was automatically updated to reflect the committed changes.