Skip to content

Conversation

@gerion-amd
Copy link

Currently supports elementwise and reduce operations, additional to matmul and reshape (within a limited set).
Could be extended to other operations later.

Copy link
Contributor

@jorickert jorickert left a comment

Choose a reason for hiding this comment

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

Thanks, especially for adding so many tests.

It looks generally good to me, but I will need to think about the reshape again, the current implementation seems to only handle some cases

@gerion-amd
Copy link
Author

It looks generally good to me, but I will need to think about the reshape again, the current implementation seems to only handle some cases

This is correct. Only reshapes that extend or lessen the shape with dimensions of rank 1 will be transformed. This seemed to be enough for real-world tests, so I left it that way, with the possibility to handle more advanced cases later. Maybe I can add a comment explaining this.

@gerion-amd gerion-amd force-pushed the gerion-amd.sink-ops-through-concat branch 2 times, most recently from b3b89b2 to 4a5a491 Compare October 2, 2025 12:55
@gerion-amd
Copy link
Author

All comments should be addressed now.

@gerion-amd gerion-amd force-pushed the gerion-amd.sink-ops-through-concat branch 5 times, most recently from 6c863cd to b6522a8 Compare October 6, 2025 09:12
Copy link
Contributor

@ehsan-toosi ehsan-toosi left a comment

Choose a reason for hiding this comment

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

I did an high-level review and checked the transformation. Just one major comment which was using trait as you and Jonas already discussed as well.
I'll do the detailed code review if I find some time today.

@gerion-amd gerion-amd force-pushed the gerion-amd.sink-ops-through-concat branch 4 times, most recently from 3fc814b to 668aed2 Compare October 8, 2025 09:44
@gerion-amd gerion-amd requested a review from ehsan-toosi October 8, 2025 09:44
@franciscofd franciscofd removed the request for review from ehsan-toosi October 8, 2025 11:22
Copy link
Contributor

@roberteg16 roberteg16 left a comment

Choose a reason for hiding this comment

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

Looks very nice! I have yet to finish the review of the reshape, I'll come back ASAP

Copy link
Contributor

@roberteg16 roberteg16 left a comment

Choose a reason for hiding this comment

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

LGTM, I would like just to have a few more reshape tests

@gerion-amd gerion-amd force-pushed the gerion-amd.sink-ops-through-concat branch 3 times, most recently from c11c500 to 33a4759 Compare October 10, 2025 19:01
@gerion-amd gerion-amd requested a review from roberteg16 October 10, 2025 19:04
@gerion-amd gerion-amd force-pushed the gerion-amd.sink-ops-through-concat branch from 33a4759 to 2fda478 Compare October 13, 2025 08:16
@gerion-amd gerion-amd force-pushed the gerion-amd.sink-ops-through-concat branch from 2fda478 to 0803705 Compare October 20, 2025 10:26
@gerion-amd gerion-amd force-pushed the gerion-amd.sink-ops-through-concat branch from 0803705 to ba93cf8 Compare October 20, 2025 10:27
@gerion-amd
Copy link
Author

Changes from 0803705 to ba93cf8:

  • Rename the variables with old and new prefix to beforeReshape and afterReshape to make it more specific that their position in the operation graph is meant
  • Add documention of the reshape method
  • Fix a bug for a reshape where the concat axis dimension is not of size 1. The approach is to also include the concat axis dimension size into the product and check for the same dimension before and afterwards.

mgehre-amd
mgehre-amd previously approved these changes Nov 5, 2025
ljfitz
ljfitz previously approved these changes Nov 6, 2025
jorickert
jorickert previously approved these changes Nov 10, 2025
@gerion-amd gerion-amd dismissed stale reviews from jorickert, ljfitz, and mgehre-amd via a712731 November 18, 2025 16:11
@gerion-amd
Copy link
Author

I added a second commit to have options to disable parts of the pass for better testing. This could be extended to optionally disable the relu operations mentioned above, if they get problematic.

@ehsan-toosi
Copy link
Contributor

Thanks.

@gerion-amd gerion-amd requested review from roberteg16 and removed request for roberteg16 November 18, 2025 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants