[AIE] Block widening register coalescing inside innerloop #406
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
niwinanto
requested review from
F-Stuckmann,
SagarMaheshwari99,
abhinay-anubola,
abnikant,
andcarminati,
gbossu,
katerynamuts,
khallouh,
konstantinschwarz,
martien-de-jong and
stephenneuendorffer
as code owners
Collaborator
Can you please update to Cycles? |
Collaborator
There was a problem hiding this comment.
This worries me a bit. We are worsening some AIE2 test results.
Collaborator
Author
There was a problem hiding this comment.
@andcarminati as discussed, I will remove this for AIE2. Also, here is the tracking ticket for AIE2P AIECC-899
Collaborator
There was a problem hiding this comment.
We may be able to take in a bit more information. If the source is within the same loop block, I think it's always better to coalesce. If the source has a lot of live intervals (indicating that it's coalesced to a lot of other registers already), it may be better not to coalesce. In that case, it may also help to split the live range by inserting a widening copy in the preheader of the loop. That allows to bring in the double register with a very short live range, (few interferences) and coalesce the now regular copy in the loop. But I'm afraid this can not be done from this target hook.
Collaborator
There was a problem hiding this comment.
Also note that this would be a natural mechanism in region-based register allocation, where the loop body would be isolated from the surroundings by these copy-in (and copy-out) moves. The inner loops then get priority, and the rest can either be coalesced or yields a low-frequency copy.
I think such a liverange splitter pass would sit naturally before register coalescing. It would guarantee that all interbank copies would be outside the inner loops. Widening copies would never be coalesced.
niwinanto
force-pushed
the
niwin.innerloop.coalesce
branch
3 times, most recently
from
9a546fc to
c18dd20
Compare
niwinanto
force-pushed
the
niwin.innerloop.coalesce
branch
from
c18dd20 to
8d6b4f7
Compare
andcarminati
reviewed
Apr 1, 2025
| AIE2P::FIFO1024RegClass.contains(Reg)); | ||
| } | ||
|
|
||
| bool AIE2PRegisterInfo::shouldCoalesce( |
Collaborator
There was a problem hiding this comment.
Maybe it is better to have this code only for AIE2P, as we are not planning to evaluate effects for AIE2 in this moment. Also, we should keep QoR results for AIE2 in a stable state.
niwinanto
force-pushed
the
niwin.innerloop.coalesce
branch
from
d4c66d4 to
1f27c64
Compare
mgehre-amd
added a commit
that referenced
this pull request
Aug 21, 2025
arith-to-emitc: Fix lowering of fptoui
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.
Coalescing widening copy from innerloop has positive impact when there is no inline spilling. However, in case of spilling, it inversely affects the stack size.