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

Cannot add more virtual registers then number physical registers to RegisterDependencyConditions #7672

Open
BradleyWood opened this issue Feb 27, 2025 · 6 comments

Comments

@BradleyWood
Copy link
Contributor

OpenJ9 recently hit this assert when generating an inline intrinsic on 32-bit.

Assertion failed at /tmp/bld_88867/bld_linux_x86/compiler/../omr/compiler/x/codegen/OMRMachine.cpp:558: considerVirtAsSpillCandidate
VMState: 0x0005ff06
	freeBestGPRegister(): could not find any GPR spill candidates for VRF_0xce6f9070

If register dependency conditions has more pre/post conditions than available number of physical registers, register assignment fails to spill registers when needed. I was able to reproduce the issue on x86_64 by adding a few extra registers live across the vectorizedHashCode() intrinsic and adding them to the RegisterDependencyConditions. Excluding these live registers from the RegisterDependencyConditions produces register spills as expected with no crash.

@0xdaryl
Copy link
Contributor

0xdaryl commented Feb 27, 2025

The register dependency condition mechanism was never designed to handle the situation where more virtual registers are requested to be assigned than there are physically available. That situation is usually (always?) a bug in the code that created the regdep because it is asking the register assigner to do the impossible.

There are situations where, for a given virtual reg, a NoReg and a real reg dependency are added to the regdep. If that situation is possible, there is a "union" function on the register dependency conditions that should merge those constraints together.

@vijaysun-omr
Copy link
Contributor

Is there a conservative fix (or a real one if that is easy enough) that comes to mind to address the issue for 32-bit while re-enabling the original code developed for the instrinsic ?

@BradleyWood
Copy link
Contributor Author

@vijaysun-omr The simplest thing is not to do loop unrolling.

@BradleyWood
Copy link
Contributor Author

@0xdaryl I think you should be able to add as many NoReg dependencies as you please to RegisterDependencyConditions

@0xdaryl
Copy link
Contributor

0xdaryl commented Feb 27, 2025

I believe that's only true for "dummy" virtual registers whose lifetime is limited to the regdep and are used to evacuate the register assigned to a virtual. I think in that instance you only need one available register of each kind to satisfy the dummy assignment.

Is that what you have in this circumstance, or are these virtual registers that are actually used in instructions?

@vijaysun-omr
Copy link
Contributor

32-bit performance is not as critical as 64-bit performance in general. So, if a simple fix is possible to keep 32-bit functional (even disabling the new optimization, or disabling the unrolling on 32-bit should be considered seriously) while re-enabling for 64-bit, then I would encourage that.

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

No branches or pull requests

3 participants