Page MenuHomePhabricator

[MLIR][Shape] Derive more concrete type for `shape.shape_of`
ClosedPublic

Authored by frgossen on Mar 23 2021, 9:32 AM.

Details

Summary

Also create all extent tensor constants with const_shape op.

Diff Detail

Event Timeline

frgossen created this revision.Mar 23 2021, 9:32 AM
frgossen requested review of this revision.Mar 23 2021, 9:32 AM

This seems as expected (capturing what is known in the non-error case when building), I'm surprised no tests needed updating - guessing as we don't have one exercising this builder. Could you perhaps add a small C++ unit test for it?

mlir/lib/Dialect/Shape/IR/Shape.cpp
1154–1155

Could you try one where you do

if (auto shapedTy = ...) {
  ...
  return ..
}
return ShapeOfOp::build(builder, result, builder.getType<ShapeType>(), arg);

? Seems like it could be simpler

1155

Nit: reduce scope by moving to just before use

1158

Hoist this out (I think it will both reduce whitespace and read easier)

herhut accepted this revision.Mar 24 2021, 3:58 AM

LGTM for the direction this takes. Please address nits.

This revision is now accepted and ready to land.Mar 24 2021, 3:58 AM
frgossen updated this revision to Diff 333820.Mar 29 2021, 4:54 AM
frgossen marked 3 inline comments as done.

Address comments

tpopp added a comment.Apr 12 2021, 2:24 AM

Is this no longer needed?

It is but there are some internal test failing that I would like to fix before landing this

frgossen edited the summary of this revision. (Show Details)Apr 16 2021, 5:45 AM
This revision now requires review to proceed.Apr 28 2021, 1:47 AM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 28 2021, 1:51 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.