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.

Sign up for GitHub

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

Jump to bottom

Fix spilling of MUL_LONG on x86 and multi-reg HWIs #73079

Merged
merged 3 commits into from
Sep 27, 2022
Merged

Fix spilling of MUL_LONG on x86 and multi-reg HWIs #73079

merged 3 commits into from
Sep 27, 2022

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Jul 29, 2022 •
edited
Loading

Fixes #73070.

Tests not added because the stress mode which makes this apparent is not really in a working shape.

No diffs.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 29, 2022
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 29, 2022
@ghost
Copy link

ghost commented Jul 29, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #73070.

Test not added because the stress mode which makes this apparent is not really in a working shape.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@SingleAccretion SingleAccretion marked this pull request as ready for review July 29, 2022 22:38
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@JulieLeeMSFT JulieLeeMSFT added this to the 8.0.0 milestone Jul 29, 2022
@JulieLeeMSFT
Copy link
Member

I set the milestone to .NET 8 because the linked issue is for 8.0.0.
FYI, we will focus on bug fixes for the next 2 weeks because RC1 snap is 8/12.

@kunalspathak
Copy link
Member

I will review this week.

@kunalspathak
Copy link
Member

/azp run runtime-coreclr superpmi-diffs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kunalspathak
Copy link
Member

No diffs.

Ah, noticed this now.

@kunalspathak
Copy link
Member

Tests not added because the stress mode which makes this apparent is not really in a working shape.

What do you mean?

@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Sep 26, 2022 •
edited
Loading

What do you mean?

The way to expose these bugs is to run something with the nodes in question under JitStressRegs=0x800, aka the "always spill" mode. However, it is very unstable and crashes the Jit even on very simple methods, as our documentation hints at.

It's very hard to expose this otherwise. It would be nice to fix the "always spill" mode, but I don't quite have the LSRA expertise necessary for that.

@kunalspathak
Copy link
Member

It would be nice to fix the "always spill" mode, but I don't quite have the LSRA expertise necessary for that.

Ok, I didn't pay attention to 0x800. Yes, we need to address it someday, I need to understand what the problems are currently.

kunalspathak
kunalspathak approved these changes Sep 27, 2022
View reviewed changes
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM. Nice cleanup.

@kunalspathak kunalspathak merged commit ee10ed1 into dotnet:main Sep 27, 2022
@SingleAccretion SingleAccretion deleted the Spilling-Fixes branch September 27, 2022 20:14
@ghost ghost locked as resolved and limited conversation to collaborators Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kunalspathak kunalspathak kunalspathak approved these changes

Assignees

@kunalspathak kunalspathak

@SingleAccretion SingleAccretion

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

Spilling does not work for some multi-reg nodes
3 participants
@SingleAccretion @JulieLeeMSFT @kunalspathak