This is an archive of the discontinued LLVM Phabricator instance.

[mlir][shape] Add dim op
ClosedPublic

Authored by jpienaar on Aug 9 2022, 7:50 AM.

Details

Summary

Convenience op that allows for simple expression of common crossing of
value/shape divide.

Diff Detail

Event Timeline

jpienaar created this revision.Aug 9 2022, 7:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 7:50 AM
jpienaar requested review of this revision.Aug 9 2022, 7:50 AM
yaochengji requested changes to this revision.Aug 9 2022, 5:52 PM

Could you add test in ops.mlir for this new op?

mlir/lib/Dialect/Shape/IR/Shape.cpp
1072

Do we need to consider more complicated situations here, where dim is not a constant-like op, but it could be folded into one?

1113

it should be dim >= st.getRank()

Further more, how do you plan to support negative dim index?

This revision now requires changes to proceed.Aug 9 2022, 5:52 PM
jpienaar updated this revision to Diff 451423.Aug 10 2022, 5:15 AM
jpienaar marked an inline comment as done.

Fix bound calculation

mlir/lib/Dialect/Shape/IR/Shape.cpp
1072

Yes but not here. That would be done as part of analysis (probably sparse data flow analysis one, we are going to rebase the TOSA shape inference one to use that framework) where one would do (potentially) expensive propagations and partial evaluations to get dims along with an inference context (well when we combine the analysis we can take advantage of SCCP's analysis state too and feed back into it, which will be nice). Here it is just for the cheap/trivial case.

1113

Negative dim index convention is not used anywhere where dim is used as operand/runtime value. I'm guessing you are thinking of Python's convention with array slicing wrt usage, similar to split_at. It is used there as its a fixed attribute so one can't do index calculation, and so if dim was an attribute here, we'd do same but for operand keeping it consistent with rest of system.

(which does remind me we didn't add sub yet as we didn't need/used the arith one as part of lowered form only).

jpienaar updated this revision to Diff 451493.Aug 10 2022, 8:41 AM

Forgot to add ops.mlir in last update

yaochengji added inline comments.Aug 10 2022, 11:01 AM
mlir/lib/Dialect/Shape/IR/Shape.cpp
1113

I guess we still need to check whether dim < 0 here. Since neither ShapeSizeType nor IndexType limit the value to be non-negative.

jpienaar added inline comments.Aug 12 2022, 9:48 AM
mlir/lib/Dialect/Shape/IR/Shape.cpp
1113

Good point, will update soon (and sorry, for some reason I didn't get a notification of your comment addressing now)

jpienaar added inline comments.Aug 12 2022, 10:02 AM
mlir/lib/Dialect/Shape/IR/Shape.cpp
1113

Tensor::DimOp also doesn't check this, that seems like bug.

jpienaar updated this revision to Diff 452212.Aug 12 2022, 10:02 AM

Verify for non-negative dims

yaochengji accepted this revision.Aug 12 2022, 10:15 AM
This revision is now accepted and ready to land.Aug 12 2022, 10:15 AM
This revision was landed with ongoing or failed builds.Aug 12 2022, 11:02 AM
This revision was automatically updated to reflect the committed changes.