Skip to content

Generalised loop tiling with examples#3039

Merged
hiker merged 6 commits into
stfc:masterfrom
mn416:mn416-generalised-tiling
Aug 1, 2025
Merged

Generalised loop tiling with examples#3039
hiker merged 6 commits into
stfc:masterfrom
mn416:mn416-generalised-tiling

Conversation

@mn416

@mn416 mn416 commented Jun 26, 2025

Copy link
Copy Markdown
Collaborator

This PR generalises LoopTiling2DTrans to LoopTilingTrans by lifting restrictions on the number of dimensions of the tile and the sizes of those dimensions (they no longer have to be uniform). For backwards compatibility, LoopTiling2DTrans remains but is now defined in terms of LoopTilingTrans.

This work was motivated by a simple example I was exploring for learning purposes: accelerating the matrix multiplication loop, which benefits from 3D tiling. This example is included in the PR, as well as a simpler example for matrix transposition, which only involves 2D tiling.

@codecov

codecov Bot commented Jun 26, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.90%. Comparing base (773f0c8) to head (691664e).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3039   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files         370      371    +1     
  Lines       52055    52085   +30     
=======================================
+ Hits        52005    52035   +30     
  Misses         50       50           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sergisiso sergisiso requested a review from hiker July 7, 2025 08:20
@sergisiso

Copy link
Copy Markdown
Collaborator

Thanks for contributing this @mn416, I gave you permissions to modify labels (you should recieve an invite). Add a test for the missing Code Coverage line and then mark this PR with the "Ready for Review" label.

@hiker hiker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for this, I have been wanting to implement something like your work forever :)

Besides the comment here (sorry, you are suffering a bit from a change of coding style and our 'you touch it, you update it' :) ), two more basic questions:

  1. Wouldn't it be a lot easier if the old 2d implementation would inherit directly from your new transpose (and just provides the right dimension sizes)
  2. You renamed the name of the tiling option (tilesize before, tiledims now). I think it would be easier if you could use the same name (tiledims). This would make it easier to replace (at some time in the future) to deprecate the old implemenation entirely. Also, if the same names would be used and you use the inherit as suggested, you remove a lot of code duplication in the validation checks for the parameters.

Comment thread examples/psyir/matmul/Makefile Outdated
Comment thread examples/psyir/matmul/omp-tile.py
Comment thread examples/psyir/matmul/omp.py
Comment thread examples/psyir/matmul/tile.py
Comment thread examples/psyir/transpose/Makefile Outdated
Comment thread src/psyclone/tests/psyir/transformations/loop_tiling_trans_test.py
Comment thread src/psyclone/psyir/transformations/loop_tiling_trans.py Outdated
Comment thread src/psyclone/psyir/transformations/loop_tiling_trans.py Outdated
Comment thread src/psyclone/psyir/transformations/loop_tiling_trans.py
Comment thread src/psyclone/tests/psyir/transformations/loop_tiling_trans_test.py
@hiker

hiker commented Jul 8, 2025

Copy link
Copy Markdown
Collaborator

@arporter, @sergisiso - I am wondering if we could consider either removing the old 2d-only tiling (which would require some small changes to existing scripts), or at least deprecating it (i.e. warning, but still works)?

@sergisiso

Copy link
Copy Markdown
Collaborator

I am wondering if we could consider either removing the old 2d-only tiling

I am in favour of removing it (without a deprecation notice/period) since it is currently not being used in any production application.

@mn416

mn416 commented Jul 8, 2025

Copy link
Copy Markdown
Collaborator Author

Thank you very much @hiker for the review. I am AFK for the rest of this week, but will respond and make the fixes next week, all being well.

@hiker

hiker commented Jul 8, 2025

Copy link
Copy Markdown
Collaborator

Thank you very much @hiker for the review. I am AFK for the rest of this week, but will respond and make the fixes next week, all being well.

No rush, I have a few days off myself as well :)

@mn416

mn416 commented Jul 15, 2025

Copy link
Copy Markdown
Collaborator Author

Thanks @hiker and @sergisiso. I've dropped loop_tiling_2d_trans.py and moved to the new way of handling transformation options. I believe I've addressed all the review comments too.

@mn416 mn416 requested a review from hiker July 15, 2025 12:16
@hiker

hiker commented Jul 18, 2025

Copy link
Copy Markdown
Collaborator

An additional comment: we discussed this PR in our last telco, and agreed that we are happy to replace the old 2d transformation long term. Since we are not certain if people are using the 2d transformation, we don't want to just break compatibility with the next release, Could you add a depreciation warning to the 2d transformation as well?

@sergisiso

Copy link
Copy Markdown
Collaborator

We just said in the previous comment that we would delete it :) I am fairly sure nobody uses it, and the update is very simple. I think we are just adding unnecessary complexity to keeping it around.

@mn416

mn416 commented Jul 18, 2025

Copy link
Copy Markdown
Collaborator Author

@sergisiso I just added it back (with a deprecation warning) before I saw your comment! But it's easy for me to delete it again. @hiker can you confirm whether you would like LoopTiling2DTrans in or out? Thanks!

@hiker

hiker commented Jul 31, 2025

Copy link
Copy Markdown
Collaborator

I'm bringing this up-to-master, and will then trigger the CI.

@hiker hiker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

CI passed, all issues addressed. Proceeding to merge.

@hiker hiker merged commit d8e0f75 into stfc:master Aug 1, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants