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

Fix crash in Beats on x86-64, minor other fixes #18131

Merged
merged 4 commits into from
Sep 11, 2023
Merged

Conversation

hrydgard
Copy link
Owner

@hrydgard hrydgard commented Sep 11, 2023

Fixes #18130 , the sc instruction failed to map the destination register.

Only use #18126 in soft-gpu mode (for now)

Add a couple of asserts.

@hrydgard hrydgard added this to the v1.16.1 milestone Sep 11, 2023
@hrydgard hrydgard merged commit 064532a into master Sep 11, 2023
18 checks passed
@hrydgard hrydgard deleted the assorted-fixes-4 branch September 11, 2023 12:52
@@ -429,6 +429,7 @@ namespace MIPSComp {
skipStore = J_CC(CC_NE);

CompITypeMemWrite(op, 32, safeMemFuncs.writeU32);
gpr.MapReg(rt, true, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, this looks wrong. You can't map in a conditional, it could corrupt other regs from the perspective of the other condition. This needs to happen before the J_CC?

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Since the reg is only written to in both branches, I actually think it should be fine here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

But hm, I'll clean it up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, because another reg might spill. If it only spills in one conditional, then that other reg is corrupted.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

A problem is that CompITypeMemWrite itself locks and unlocks regs, so I can't reliably do it that way, can I?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hm, I think the actual bug is in CompITypeMemWrite, in the gpr.R(rt).IsImm() case, since normally we'd end up with rt being mapped coming out of it.

I'll fix that.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Well, not so much a bug as CompITypeMemWrite just not being designed to leave rt writable, fixable with a flag.

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.

Beats (NPUG80060) regression
2 participants