[AIE2P] Enable up-combining for VLD + VCONV #392
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andcarminati
requested review from
F-Stuckmann,
SagarMaheshwari99,
abhinay-anubola,
abnikant,
gbossu,
katerynamuts,
khallouh,
konstantinschwarz,
martien-de-jong,
niwinanto and
stephenneuendorffer
as code owners
Collaborator
There was a problem hiding this comment.
Could you be a bit more precise? I think this is based on regression figures for AIE2 at a very particular time, state of the combines, state of the benchmark suites, etc. If there's a specific feature of AIE2P or AIE2 that is driving this, please mention it. If we can derive it from specific instruction characteristics of the conv or the load instructions, consider testing for that specific instruction feature.
Collaborator
Author
There was a problem hiding this comment.
We have this for other patterns in aie2, but not for this. I would prefer to not change the behavior for a target that we are not testing anymore, independent on the reason. In that time, a bunch of spill regression related to accumulator registers were found and I don't have all those results anymore.
Collaborator
There was a problem hiding this comment.
nit: I would reverse the condition of this nested if, having an early return false and no else.
Collaborator
There was a problem hiding this comment.
Very general: parent checks are the cheapest one in town and should absolutely be done first. And I mean first-first, perhaps even before calling the select function.
Collaborator
Author
There was a problem hiding this comment.
Yeah, we could optimize this selection framework as a whole. I created a ticket to review all cases.
Collaborator
There was a problem hiding this comment.
add 'by moving the VCONV up to the load.'
Collaborator
There was a problem hiding this comment.
Actually, I think advancing the conversion is in general better and easier. We usually want to schedule loads early due to long latency. Can we try it first?
andcarminati
force-pushed
the
andreu.vld.vconv.up
branch
from
c40983b to
aadb4c6
Compare
andcarminati
commented
Mar 24, 2025
Collaborator
Author
There was a problem hiding this comment.
This is not correct, addressing register is defined after.
andcarminati
force-pushed
the
andreu.vld.vconv.up
branch
from
aadb4c6 to
2407fe3
Compare
Collaborator
Author
|
QoR numbers: For |
andcarminati
force-pushed
the
andreu.vld.vconv.up
branch
from
2407fe3 to
cd33344
Compare
martien-de-jong
approved these changes
Mar 31, 2025
Collaborator
martien-de-jong
left a comment
martien-de-jong
left a comment
There was a problem hiding this comment.
LGTM. Although the 'only for aie2p' condition remains mysterious.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This catches some missing opportunities in GEMM.
FYI: @martien-de-jong .