Skip to content

Remove TR_X87_Mask guarded code #22103

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

Merged
merged 1 commit into from
Jul 18, 2025

Conversation

Spencer-Comin
Copy link
Contributor

@Spencer-Comin Spencer-Comin commented Jun 16, 2025

X87 registers are no longer allocated.

Requires coordinated merge with eclipse-omr/omr#7799

X87 registers are no longer allocated.

Signed-off-by: Spencer Comin <[email protected]>
@Spencer-Comin
Copy link
Contributor Author

Internal testing here

@Spencer-Comin Spencer-Comin requested a review from 0xdaryl June 16, 2025 22:57
@0xdaryl
Copy link
Contributor

0xdaryl commented Jun 16, 2025

@BradleyWood : please review

@@ -71,14 +71,11 @@ TR::Snippet *TR::X86MemImmSnippetInstruction::getSnippetForGC()
void TR::X86MemImmSnippetInstruction::assignRegisters(TR_RegisterKinds kindsToBeAssigned)
{
TR::X86MemImmInstruction::assignRegisters(kindsToBeAssigned);
if (kindsToBeAssigned & (TR_X87_Mask | TR_FPR_Mask))
if (kindsToBeAssigned & TR_FPR_Mask)
Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm if TR_X87_Mask is ever set? If its not, then this probably doesn't depend on eclipse-omr/omr#7799 and we can avoid a coordinated merge situation.

Copy link
Member

Choose a reason for hiding this comment

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

It would avoid a coordinated merge, but I think this pull request would need to be merged before eclipse-omr/omr#7799 could be merged.

Copy link
Contributor Author

@Spencer-Comin Spencer-Comin Jul 16, 2025

Choose a reason for hiding this comment

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

@BradleyWood I don't see anywhere TR_X87_Mask is set. To be safe, I started an internal build without eclipse-omr/omr#7799. All the tests passed; I'd say this doesn't depend on eclipse-omr/omr#7799.

@0xdaryl 0xdaryl self-assigned this Jul 17, 2025
@Spencer-Comin
Copy link
Contributor Author

Given testing in #22103 (comment), we don't need a coordinated merge with eclipse-omr/omr#7799

@0xdaryl
Copy link
Contributor

0xdaryl commented Jul 17, 2025

Jenkins test sanity xlinux,win32 jdk8

@0xdaryl 0xdaryl merged commit 1146953 into eclipse-openj9:master Jul 18, 2025
9 checks passed
{
TR::UnresolvedDataSnippet *snippet = getMemoryReference()->getUnresolvedDataSnippet();
if (snippet)
{
if (kindsToBeAssigned & TR_X87_Mask)
snippet->setNumLiveX87Registers(cg()->machine()->fpGetNumberOfLiveFPRs());

if (kindsToBeAssigned & TR_FPR_Mask)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this test now redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the if (kindsToBeAssigned & TR_FPR_Mask) test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's (now) the same test as on line 74, which we know is true here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove that in a future PR together with some other dead x87 code

@Spencer-Comin Spencer-Comin deleted the remove-TR_X87_Mask branch August 6, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants