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

Add donated scratch regs to dependency list only when they are used #7628

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthewhall2
Copy link
Contributor

@matthewhall2 matthewhall2 commented Jan 27, 2025

Adds msrUsedDonated flag to mark that a donated scratch register has been used.

If using findOrCreateScratchRegister within ICF and there are no free registers, cg->allocateRegister is called causing a fatal assertion
TR_ASSERT_FATAL(false, "Attempting to assign a register (%s) inside ICF", getRegisterName(targetRegister, self()->cg()));
Thus, we have to donate registers before entering an internal control flow region where we request a scratch register.

For example, there are cases where we need at most 2 scratch registers, and the use of the second register is guarded by an if statement (i.e. in the java superclass test on Z):

void genSuperclassTest(..., ScratchRegisterManager *srm, ...) {
    ...
    TR::Register *castClassDepthReg = NULL;
    if (castClassDepth == -1) { // we do not know the depth at compile time
        castClassDepthReg = srm->findOrCreateScratchRegister();
    }
    ....
    TR::Register *objClassDepthReg = srm->findOrCreateScratchRegister();
}

Since the superclass test is generated inside an ICF region, we need to allocate the 2 registers before entering the region. But when castClassDepth != -1, we never use one of the donated registers. Adding this unused register to the dependency list results in an extra real register being allocated.

This change allows the Scratch Register Manager to treat donated and non-donated registers in the same way: they are only added to the dependency list if they are actually used. This way, we allow the user to set the capacity of the scratch register manager to the maximum amount they need, and donate how ever many they want (up to the maximum), without worrying about whether all donated registers are used in all cases.

Adds msrDonateUsed flag to mark that a donated scratch register has been
used.
addScratchRegistersToDependencyList will not add a donated regsiter to
the dependency list unless the msrDonateUsed flag is set.

This fixes an issue where a register is donated but not used and is
still added to be dependency list, causing an extra register to be
assigned at labels marked with endInternalControlFlow

Signed-off-by: Matthew Hall <matthew.hall3@outlook.com>
@matthewhall2 matthewhall2 marked this pull request as ready for review January 27, 2025 21:42
@matthewhall2
Copy link
Contributor Author

matthewhall2 commented Jan 27, 2025

most tests passing. I've gone through the unstable tests and similar failures were seen in recent nightly builds. Re-running them in Grinder to make sure.
please review @r30shah

Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

Hi @matthewhall2 , thanks a lot for the change. It is bit unfortunate that in case of checkCast evaluator, you do enter a case where ICF is entered and SRM you have created has not been used to its capacity, causing you to allocate a register inside ICF. I have been thinking about the problem you are having.

Although, the change you are adding here would make it easier to solve problem you are having where SRM is used in checkCast evaluator on Z where you do not need all register in all cases, I do think that a good written evaluator, should ensure that they are allocating by themselves/through SRM only number of registers they are going to use in the evaluator.

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.

2 participants