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.

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

(Closes #2774) Refactor matmul2code to allow for more permutations of full ranges #2795

Merged
merged 26 commits into from
Nov 22, 2024

Conversation

DrTVockerodtMO
Copy link
Collaborator

Fixes #2774.

There was a hardcoded standard in PSyclone where for matrix multiplications, the first one or two indices must be full ranges (i.e.: the Fortran would be written as matrix(:,:,...) or vector(:,...)). However, Fortran can perform matrix multiplications with the full ranges in different orderings, as long as the shape conforms.

When recent LFRic changes re-ordered some of the operator index accesses, this shifted the full ranges into forbidden permutations.

This change addresses this issue by allowing more permutations that produce valid Fortran matmul code.

Description of changes
Added a new private method _get_full_range_split to matmul2code_trans.py, which splits information about full range and non full range nodes as well as performs some validation. For a given array, the method iterates over the children, and checks whether or not they are Range nodes. If the Range node is a full range, we append its position in the array to a list. If not, we throw an error. If it is not a Range node then its position and the node itself are appended to two different lists.

After the children are iterated over, if we have more than two full ranges then this array is not a valid input for matmul, so we throw an error. Otherwise, return the two lists containing the positions of the full range and non full range nodes in the array, as well as the non full range node list.

Some further validation is performed to ensure that the first matrix involved in the matmul has exactly two full ranges, and that the second matrix has one or two (since it can be a vector or matrix).

The orderings and list of non full range nodes are fed to the modified _create_matrix_ref to make the matrix reference.

Testing
A new test test_apply_matmat_reordered has been added which tests the expected behaviour directly, by feeding the PSyIR some Fortran code with spatially conforming but re-ordered (with respect to the old standard) matrices. Other tests will be modified since a lot of them tested aspects of the old standard that no longer apply.

@DrTVockerodtMO DrTVockerodtMO added bug enhancement LFRic Issue relates to the LFRic domain adjoint labels Nov 20, 2024
@DrTVockerodtMO DrTVockerodtMO self-assigned this Nov 20, 2024
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.88%. Comparing base (4cb5070) to head (54f5cb8).
Report is 27 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2795   +/-   ##
=======================================
  Coverage   99.88%   99.88%           
=======================================
  Files         356      356           
  Lines       49481    49507   +26     
=======================================
+ Hits        49423    49451   +28     
+ Misses         58       56    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@DrTVockerodtMO DrTVockerodtMO marked this pull request as ready for review November 20, 2024 14:23
Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Good stuff @DrTVockerodtMO.
I think the fix/extension is good. Just a few minor things to tidy up and I've requested an extra test.
Although not strictly your fault, I note that the associated test file does not give 100% coverage - lines 424-425 are missed. You can see this by doing pytest --cov psyclone.psyir.transformations.intrinsics.matmul2code_trans -- cov-report term-missing psyir/transformations/intrinsics/matmul2code_trans_test.py. Could you take a look?
Please could you also update the class docstring of apply() to mention the restriction to full ranges and simple arrays (i.e. structures of arrays etc.).
I've fired off the integration tests so these can run in the meantime.

@arporter arporter added reviewed with actions Release Planning and creating PSyclone releases and removed under review labels Nov 21, 2024
@arporter
Copy link
Member

Just a note to say integration tests were OK (barring the expected failures).

@DrTVockerodtMO
Copy link
Collaborator Author

Thanks Terry. Apologies for the confusion over the strings (KGOs) in the tests. Apart from that, looking at the missed lines made me realise that we still seem to have an implicit assumption in the code about the form of the result array that we support. We can either generalise this too or we can make the assumption explicit by checking for it in validate. The good news is that the integration tests were all fine :-)

Addressed in 4629275 and 703c3e1 which now has a different permutation of the result matrix in the calculation.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Thanks Terry. Looking very good now.
I think the only thing left is to refactor the validation as we have quite a bit of duplication there now (the part that checks the number of ranges).
Once that's done this I'll proceed to merge!

@arporter
Copy link
Member

(Just a note to say that the test file now gives 100% coverage of the transformation file :-) )

…ase, which should fix code coverage tests failing.
Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

All good now - thanks for removing the duplication.
There are no unaccounted-for coverage changes.
CI is green. Integration tests were previously green.
Will proceed to merge.

@arporter arporter merged commit 38ba992 into master Nov 22, 2024
12 of 13 checks passed
@arporter arporter deleted the 2774_matmul2code_trans_refactor branch November 22, 2024 14:12
@DrTVockerodtMO DrTVockerodtMO mentioned this pull request Nov 22, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adjoint bug enhancement LFRic Issue relates to the LFRic domain ready to be merged Release Planning and creating PSyclone releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LFRic] [PSyAD] Change in operator index ordering does not match PSyAD's matmul standard
2 participants