Page MenuHomePhabricator

Moved getStaticLoopRanges and getStaticShape methods to LinalgInterfaces.td to add static shape verification
ClosedPublic

Authored by inho9606 on Mar 7 2021, 11:56 PM.

Details

Summary

It is to use the methods in LinalgInterfaces.cpp for additional static shape verification to match the shaped operands and loop on linalgOps. If I used the existing methods, I would face circular dependency linking issue. Now we can use them as methods of LinalgOp.

Diff Detail

Event Timeline

inho9606 created this revision.Mar 7 2021, 11:56 PM
inho9606 requested review of this revision.Mar 7 2021, 11:56 PM
ftynse added a subscriber: ftynse.Mar 8 2021, 2:06 AM

Please provide a rationale for making this change in the commit description. Why is it necessary? Is there any op that will customize the behavior and why can't it do so by overloading the functions instead?

hanchung requested changes to this revision.Mar 8 2021, 4:23 AM

Please provide a rationale for making this change in the commit description. Why is it necessary? Is there any op that will customize the behavior and why can't it do so by overloading the functions instead?

Inho is going to add static shape verification for Linalg structured ops. E.g., verify if the shapes match the loops for conv op. He is going to use these methods in the change. To reuse methods in
verifyStructuredOpInterface, we have to move methods out of Utils.h. Otherwise, there will be circular dependency. We already have some similar interface methods like getShapesToLoopsMap, etc. I think it makes sense to move static version to interface methods.

Inho, could you provide more context to commit description and fix the title? Please also fix the build.

The below link has an overview of writing a good change descriptions, please take a look: https://google.github.io/eng-practices/review/developer/cl-descriptions.html

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
197–234

I think they are not for "loop types handling". Maybe move them to "Linalg generalization hooks."?

This revision now requires changes to proceed.Mar 8 2021, 4:23 AM
  1. Updating D98163: #Enter a commit message. # Moved fectored getStaticLoopRanges and getStaticShape methods to LinalgInterfaces.td #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Moved getStaticShape and getStaticLoopRanges method to Linalg Generlization Hooks part.n LinalgInterfaces.td
This is to use these methods in LinalgInterfaces.cpp for additional static shape verification to match the shapes and loops. If I used existing methods, circular dependency issue happened. Now we can use the methods with linalgOp.getStaticLoopRanges().

inho9606 retitled this revision from #Enter a commit message. # Moved fectored getStaticLoopRanges and getStaticShape methods to LinalgInterfaces.td to Moved getStaticLoopRanges and getStaticShape methods to LinalgInterfaces.td to add static shape verification.Mar 8 2021, 6:08 PM
inho9606 edited the summary of this revision. (Show Details)

Please provide a rationale for making this change in the commit description. Why is it necessary? Is there any op that will customize the behavior and why can't it do so by overloading the functions instead?

Sorry for confusing commit message. It was my first change request here, so the interface was not familiar for me. I didn't know how to update the revision at the moment, now I updated it.
Thanks

Uh,, Sorry I removed @ftynse from subscribers,, but don't know why it happened..

inho9606 added inline comments.Mar 8 2021, 6:49 PM
mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
197–234

I moved it, could you look at it again?

hanchung requested changes to this revision.Mar 8 2021, 11:35 PM
hanchung added inline comments.
mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
1143

Let's follow other places -- adding a blank line right before the comment.

1146

Please also update the comment. In the past, we did return -1 which indicated to dynamic dim. Now we have ShapeType::kDynamicSize, so let's use it in the comment instead.

1153–1159

Do we need to get the operation and cast to LinalgOp? I thought getShapedOperands is a class method which can be access directly?

1166

ditto

1174

ditto

This revision now requires changes to proceed.Mar 8 2021, 11:35 PM
  1. Updating D98163: Moved getStaticLoopRanges and getStaticShape methods to LinalgInterfaces.td to add static shape verification #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Reflected request changes

inho9606 added inline comments.Mar 9 2021, 2:01 AM
mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
1153–1159

Oh right, I had to check it beforehand,, thanks. And also I think linalgOp is not used in getStaticLoopRanges method, so I removed it.

hanchung added inline comments.Mar 9 2021, 4:17 AM
mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
1127

I think this -1 should also be ShapeType::kDynamicSize?

  1. Updating D98163: Moved getStaticLoopRanges and getStaticShape methods to LinalgInterfaces.td to add static shape verification #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment

Changed -1 to ShapedType::kDynamicSize in comment

hanchung added inline comments.Mar 10 2021, 1:19 AM
mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
1127–1128

It is too long, we have to format it a bit.

Returns the statically-known loop ranges. Composes
`getShapesToLoopsMap()` with the result of `getStaticShape`.
Returns None if `getShapesToLoopsMap()` fails. Returns
ShapeType::kDynamicSize for non-statically-known loop ranges.
  1. Updating D98163: Moved getStaticLoopRanges and getStaticShape methods to LinalgInterfaces.td to add static shape verification #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

reformating

  1. Updating D98163: Moved getStaticLoopRanges and getStaticShape methods to LinalgInterfaces.td to add static shape verification #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Reverted the test file

hanchung accepted this revision.Mar 10 2021, 3:52 AM
This revision is now accepted and ready to land.Mar 10 2021, 3:52 AM
This revision was landed with ongoing or failed builds.Mar 10 2021, 4:06 AM
This revision was automatically updated to reflect the committed changes.
Harbormaster completed remote builds in B93044: Diff 329581.