Page MenuHomePhabricator

Enables inferring return types for Shape op if possible
ClosedPublic

Authored by Chia-hungDuan on May 15 2021, 8:19 PM.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.May 15 2021, 8:19 PM
Chia-hungDuan requested review of this revision.May 15 2021, 8:19 PM
jpienaar added inline comments.May 17 2021, 2:16 PM
mlir/lib/Dialect/Shape/IR/Shape.cpp
425

The number of operands may not have been verified here yet, lets return failure if there are not 2 operands

436

Lets check here that it is one of the given types just as haven't verified (lets chat about that later this week too)

Seems like a few of these could be deduped, which would help if we have shared functions here to refactor later.

Chia-hungDuan marked an inline comment as done.

Address review comment and fix the isCompatibleReturnType of MaxOp and MinOp

Add one more type check for ShapeOfOp

Chia-hungDuan marked an inline comment as done.May 18 2021, 4:25 AM
Chia-hungDuan added inline comments.
mlir/lib/Dialect/Shape/IR/Shape.cpp
425

Added check for size of operands

jpienaar added inline comments.May 26 2021, 3:43 PM
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
31–33

We don't have a formatgter for this yet, but the convention we've been following is to have : ShapeOp on the line in line with the def

def Shape_AddOp : Shape_Op<"add", [

in some cases the [ and traits on the next, but mostly it opens on the former.

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

May be a case of missing change upload again , I'll double check

438

Mmm, this seems to be the same as

return l.front().isa<SizeType, IndexType>() || r.front().isa<SizeType, IndexType>() ?

1213

Aren't these the only two allowed types for this operand?

Chia-hungDuan marked an inline comment as done.

Remove the type/size check for operands in inferReturnTypes and addressed the review comments

Chia-hungDuan added inline comments.Jul 26 2021, 4:20 PM
mlir/lib/Dialect/Shape/IR/Shape.cpp
438

I guess you were saying
return l.front().isa<SizeType, IndexType>() && r.front().isa<SizeType, IndexType>()
Done.

1213

Yes,

let results = (outs Shape_ShapeOrSizeType:$result);
jpienaar accepted this revision.Jul 29 2021, 6:38 PM
jpienaar added inline comments.
mlir/include/mlir/Dialect/Shape/IR/ShapeOps.td
54

Perhaps (here and below), Returns when two result types are compatible for this op; method used by InferTypeOpInterface ?

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

Could we make a static helper function and just call it from the others? (this seems like the same code below)

1203

If we had

lhs.isa<IndexType, SizeType> && rhs.isa<SizeType, IndexType>()

but lhs.isa<IndexType> && rhs.isa<SizeType>, then this would fail, but seems fine/non-error. Or am I missing it?

1275

Could we just use operands[0].getType() here rather than getting one?

This revision is now accepted and ready to land.Jul 29 2021, 6:38 PM
Chia-hungDuan marked 2 inline comments as done.

Address review comment

Chia-hungDuan added inline comments.Aug 9 2021, 6:53 PM
mlir/lib/Dialect/Shape/IR/Shape.cpp
1203
let results = (outs Shape_ShapeOrSizeType:$result);

The result of JoinOp is Shape_ShapeOrSizeType. Or do we need to update the semantic of JoinOp to make it also be able to produce IndexType?

1275

It should be isa<ShapeType> here. Fixed

Rebase and minor update commit message

This revision was landed with ongoing or failed builds.Aug 18 2021, 2:38 PM
This revision was automatically updated to reflect the committed changes.