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 operation data type based on its params for Corr2D #63

Merged
merged 16 commits into from
Aug 8, 2022
Merged

Infer operation data type based on its params for Corr2D #63

merged 16 commits into from
Aug 8, 2022

Conversation

ArtemSkrebkov
Copy link
Contributor

@ArtemSkrebkov ArtemSkrebkov commented Aug 1, 2022 •
edited
Loading

Motivation

Opens

  • Is it feasible to make output to be a returned value?
    • I gave a try but had to get back to a parameter for the output since I didn't manage to expose returned value via C interface

ArtemSkrebkov
ArtemSkrebkov commented Aug 1, 2022
View reviewed changes
lib/Conversion/LowerDIP/LowerDIPPass.cpp Outdated Show resolved Hide resolved
lib/Dialect/DIP/DIPOps.cpp Outdated Show resolved Hide resolved
@ArtemSkrebkov
Add a operand type check for Corr2D op and a test
566afc8 
  - Make sure that input, kernel, output and constant have the same
    value and use as inferred type
  - Adding a negative lit test to check params of the op
meshtag
meshtag suggested changes Aug 1, 2022
View reviewed changes
Copy link
Member

@meshtag meshtag left a comment

Choose a reason for hiding this comment

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

Thanks a lot for introducing llvm style tests for the DIP dialect!

Overall, I don't see any major issue with the implementation mechanism and I think you can continue with the rest of the implementation for covering remaining types.

tests/Dialect/DIP/correlation2D_invalid_type.mlir Outdated Show resolved Hide resolved
lib/Conversion/LowerDIP/LowerDIPPass.cpp Outdated Show resolved Hide resolved
include/Utils/DIPUtils.h Outdated Show resolved Hide resolved
@ArtemSkrebkov ArtemSkrebkov marked this pull request as ready for review August 2, 2022 14:24
@ArtemSkrebkov
Copy link
Contributor Author

@meshtag

Please take a look one more time. f64 test might fail. I will fix it. But the other is ready for review.

meshtag
meshtag suggested changes Aug 2, 2022
View reviewed changes
Copy link
Member

@meshtag meshtag left a comment

Choose a reason for hiding this comment

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

I'll build this branch and have a more detailed look once the issue with f64 test is fixed.

include/Utils/DIPUtils.h Outdated Show resolved Hide resolved
lib/Conversion/LowerDIP/LowerDIPPass.cpp Outdated Show resolved Hide resolved
lib/Dialect/DIP/DIPOps.cpp Outdated Show resolved Hide resolved
@ArtemSkrebkov
Copy link
Contributor Author

f64 can work as well, so ready for one more round of review.

meshtag
meshtag suggested changes Aug 3, 2022
View reviewed changes
tests/Dialect/DIP/correlation2D_f32.mlir Outdated Show resolved Hide resolved
tests/Dialect/DIP/correlation2D_invalid_type.mlir Show resolved Hide resolved
lib/Conversion/LowerDIP/LowerDIPPass.cpp Outdated Show resolved Hide resolved
include/Utils/DIPUtils.h Show resolved Hide resolved
ArtemSkrebkov and others added 3 commits August 4, 2022 11:12
@ArtemSkrebkov
Fix more review comment
8d556be 
  - extend correlation2d_invalid_type test to cover a condition for supported types
  - add comments for utility functions
@ArtemSkrebkov
Remove insertZeroConstantOp from LowerDIPPass
b143870 
  - it is present in DIPItility.h
@meshtag
Trivial changes
b3dfbb5
meshtag
meshtag approved these changes Aug 4, 2022
View reviewed changes
Copy link
Member

@meshtag meshtag left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! (/cc @zhanghb97 )
Thanks @ArtemSkrebkov !

@ArtemSkrebkov ArtemSkrebkov mentioned this pull request Aug 5, 2022
Open
@zhanghb97
Copy link
Member

Thanks @ArtemSkrebkov @meshtag !

@zhanghb97 zhanghb97 merged commit 1e86d5a into buddy-compiler:main Aug 8, 2022
Ris-Bali pushed a commit to Ris-Bali/buddy-mlir that referenced this pull request Aug 27, 2022
@ArtemSkrebkov @meshtag @Ris-Bali
[DIP] Infer operation data type based on its params for Corr2D (buddy…
303d505 
…-compiler#63)

* Add lit-test for correlation2D

* Add correlation i32 test

* Add support for i32 correlation 2d

* Enable default attribute printing for DIP

* Add a check for compatbility return type

* Update lit test to find mlir cpu utils

* Make output to be param for Corr2d

  - having it as a return value causes to segfault correlation2D sample
    - it is not clear how to return MemRef from C-interface

* Mark attributes with <> in dip.mlir

* Add a operand type check for Corr2D op and a test

  - Make sure that input, kernel, output and constant have the same
    value and use as inferred type
  - Adding a negative lit test to check params of the op

* Fix review comments

* Add support for i8,i64,f64

* Fix correlation2D_f64 test

 * by constructing F64 correctly

* Fix review comment and formatting issues

* Fix more review comment

  - extend correlation2d_invalid_type test to cover a condition for supported types
  - add comments for utility functions

* Remove insertZeroConstantOp from LowerDIPPass

  - it is present in DIPItility.h

* Trivial changes

Co-authored-by: meshtag <prathameshtagore@gmail.com>
WLFJ pushed a commit to WLFJ/buddy-mlir that referenced this pull request Sep 20, 2022
@ArtemSkrebkov @meshtag
[DIP] Infer operation data type based on its params for Corr2D (buddy…
39b1b78 
…-compiler#63)

* Add lit-test for correlation2D

* Add correlation i32 test

* Add support for i32 correlation 2d

* Enable default attribute printing for DIP

* Add a check for compatbility return type

* Update lit test to find mlir cpu utils

* Make output to be param for Corr2d

  - having it as a return value causes to segfault correlation2D sample
    - it is not clear how to return MemRef from C-interface

* Mark attributes with <> in dip.mlir

* Add a operand type check for Corr2D op and a test

  - Make sure that input, kernel, output and constant have the same
    value and use as inferred type
  - Adding a negative lit test to check params of the op

* Fix review comments

* Add support for i8,i64,f64

* Fix correlation2D_f64 test

 * by constructing F64 correctly

* Fix review comment and formatting issues

* Fix more review comment

  - extend correlation2d_invalid_type test to cover a condition for supported types
  - add comments for utility functions

* Remove insertZeroConstantOp from LowerDIPPass

  - it is present in DIPItility.h

* Trivial changes

Co-authored-by: meshtag <prathameshtagore@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meshtag meshtag meshtag approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ArtemSkrebkov @zhanghb97 @meshtag