-
Notifications
You must be signed in to change notification settings - Fork 28
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.
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
add {min,max}val2code transformations (towards #2105) #2148
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2148 +/- ##
=======================================
Coverage 99.85% 99.85%
=======================================
Files 339 342 +3
Lines 46069 46144 +75
=======================================
+ Hits 46000 46075 +75
Misses 69 69
☔ View full report in Codecov by Sentry. |
This PR needs the HUGE and TINY intrinsics (or equivalent) to set initial values. PR #2130 has now added these so we can continue with this PR. |
Ready for first review from @arporter or @sergisiso |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are complicated transformations! I've only looked at maxval2code and the mms base class so far and have a lot of questions so I'm stopping here for now.
All the tests and examples are OK with compilation.
The updated (thanks) UG builds fine.
src/psyclone/psyir/transformations/intrinsics/maxval2code_trans.py
Outdated
Show resolved
Hide resolved
src/psyclone/psyir/transformations/intrinsics/maxval2code_trans.py
Outdated
Show resolved
Hide resolved
src/psyclone/psyir/transformations/intrinsics/maxval2code_trans.py
Outdated
Show resolved
Hide resolved
src/psyclone/psyir/transformations/intrinsics/maxval2code_trans.py
Outdated
Show resolved
Hide resolved
src/psyclone/psyir/transformations/intrinsics/mms_base_trans.py
Outdated
Show resolved
Hide resolved
Ready for next review from @arporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job. It's a nice implementation now. Despite the number of comments this is nearly there.
I was about to say I'll fire off the integration tests but there's no point as they don't actually use these transformations. That should probably be a separate PR.
I'll do it next time though to check the compilation of the tests.
I've checked everything with pylint.
Ref. guide builds without new warnings.
src/psyclone/psyir/transformations/intrinsics/maxval2code_trans.py
Outdated
Show resolved
Hide resolved
src/psyclone/psyir/transformations/intrinsics/mms_base_trans.py
Outdated
Show resolved
Hide resolved
src/psyclone/tests/psyir/transformations/intrinsics/minval2code_trans_test.py
Show resolved
Hide resolved
src/psyclone/tests/psyir/transformations/intrinsics/maxval2code_trans_test.py
Show resolved
Hide resolved
src/psyclone/tests/psyir/transformations/intrinsics/maxval2code_trans_test.py
Show resolved
Hide resolved
src/psyclone/tests/psyir/transformations/intrinsics/sum2code_trans_test.py
Show resolved
Hide resolved
src/psyclone/tests/psyir/transformations/intrinsics/sum2code_trans_test.py
Show resolved
Hide resolved
Ready for next review from @arporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nearly there.
Integration tests are still underway.
I just have one outstanding query about the pair of tests that you say are doing different things...
Ready for next review from @arporter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requested change has been made.
Integration tests were all fine.
Ref. guide has no more warnings that on master.
Will proceed to merge.
Adds two new transformations which replace minval and maxval intrinsics with equivalent code. The requirements for these are similar to the existing sum2code transformation.