Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Infer bounds for Reduce op #737

Merged
merged 4 commits into from
Dec 16, 2022
Merged

Infer bounds for Reduce op #737

merged 4 commits into from
Dec 16, 2022

Conversation

smit-hinsu
Copy link
Contributor

Extended reduce op shape function to also propagate bounds for dimensions that are not reduced.

Also, moved verifyCompatibleShapeWithBounds to public in Base.h

cc @zhouxin913 for review

@zhouxin913
Copy link
Contributor

Hi Smit, as I have just submit #401, this PR will have conflict with HEAD. You may consider rebase now or any time later: my PR only moves some checks in a util function (For ReduceOp it is verifyReduceOpInputsAndInferShape() without any logical change.
This is not a blocker, I will continue my review and leave comments, just FYI.

@smit-hinsu
Copy link
Contributor Author

Hi Smit, as I have just submit #401, this PR will have conflict with HEAD. You may consider rebase now or any time later: my PR only moves some checks in a util function (For ReduceOp it is verifyReduceOpInputsAndInferShape() without any logical change. This is not a blocker, I will continue my review and leave comments, just FYI.

Rebased this.

@zhouxin913 zhouxin913 mentioned this pull request Dec 12, 2022
Open
zhouxin913
zhouxin913 reviewed Dec 12, 2022
View reviewed changes
Copy link
Contributor

@zhouxin913 zhouxin913 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a tiny comment in tests.

Thanks for your fix in https://reviews.llvm.org/D139271: with which we can add encoding when init inferred type even with ShapedTypeComponents interface.

Besides, this PR makes issue #394 more complicated, which can be addressed at that issue (I have left a comment there, so that this PR is good to go)
Assign to @burmako for a second review.

stablehlo/tests/infer_stablehlo.mlir Show resolved Hide resolved
@zhouxin913 zhouxin913 assigned burmako and unassigned zhouxin913 Dec 12, 2022
@burmako burmako assigned zhouxin913 and unassigned burmako Dec 15, 2022
@zhouxin913 zhouxin913 merged commit a0c8588 into openxla:main Dec 16, 2022
@burmako burmako added the Migrate to MHLO PR that needs to be migrated to MLIR-HLO label Dec 18, 2022
@copybara-service copybara-service bot mentioned this pull request Dec 28, 2022
Closed
copybara-service bot pushed a commit to tensorflow/mlir-hlo that referenced this pull request Dec 29, 2022
[StableHLO to MHLO] Infer bounds for Reduce op
b23cbc2 
Related StableHLO PR:
openxla/stablehlo#737.

Update tests only.

PiperOrigin-RevId: 498419074
copybara-service bot pushed a commit to openxla/xla that referenced this pull request Dec 29, 2022
@copybara-github
[StableHLO to MHLO] Infer bounds for Reduce op
e0e11b4 
Related StableHLO PR:
openxla/stablehlo#737.

Update tests only.

PiperOrigin-RevId: 498419074
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Dec 29, 2022
@tensorflower-gardener
[StableHLO to MHLO] Infer bounds for Reduce op
fc702df 
Related StableHLO PR:
openxla/stablehlo#737.

Update tests only.

PiperOrigin-RevId: 498419074
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhouxin913 zhouxin913 zhouxin913 left review comments

@burmako burmako burmako approved these changes

Assignees

@zhouxin913 zhouxin913

Labels
Migrate to MHLO PR that needs to be migrated to MLIR-HLO Type inference
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smit-hinsu @zhouxin913 @burmako