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

Map correct fmt for mov instruction #79174

Merged
merged 2 commits into from
Dec 6, 2022

Conversation

kunalspathak
Copy link
Member

With #78879, we most likely exposed a problem in emitter, where mov would never go through emitIns_R_A.

I surveyed all the places that calls emitHandleMemOp() and hence emitMapFmtForIns() and the only place where mov can now end up to pass through emitIns_R_A() with wrong fmt. The format that would pass is IF_RRW_ARD (read/write) but it should be IF_RWR_ARD (since mov just writes to the register). The fix is to add a check in emitMapFmtForIns() to convert to right format.

Fixes: #79132

@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 Dec 2, 2022
@ghost ghost assigned kunalspathak Dec 2, 2022
@ghost
Copy link

ghost commented Dec 2, 2022

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

Issue Details

With #78879, we most likely exposed a problem in emitter, where mov would never go through emitIns_R_A.

I surveyed all the places that calls emitHandleMemOp() and hence emitMapFmtForIns() and the only place where mov can now end up to pass through emitIns_R_A() with wrong fmt. The format that would pass is IF_RRW_ARD (read/write) but it should be IF_RWR_ARD (since mov just writes to the register). The fix is to add a check in emitMapFmtForIns() to convert to right format.

Fixes: #79132

Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Member Author

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib @BruceForstall

@BruceForstall BruceForstall merged commit bd768c7 into dotnet:main Dec 6, 2022
@jakobbotsch
Copy link
Member

Any idea why my jitstress runs in #78879 didn't hit this?

@kunalspathak
Copy link
Member Author

Any idea why my jitstress runs in #78879 didn't hit this?

That's an interesting question. I tried locally to repro on the commit of #78879 and it repros for me. I also tried searching for logs of jitstress that was ran, but don't see that test anywhere. I am not sure why it didn't hit there.

@jakobbotsch
Copy link
Member

Yeah, I don't know either. The testing I did was slightly different than normal jitstress: I disabled all heuristics in if-conversion and allowed if-conversion to convert all eligible if's into GT_SELECT, instead of only 25% of them (as in normal stress). But given the extensive failures in jitstress2 it's still very odd that I hit none of those even if this would be slightly different.

@jakobbotsch
Copy link
Member

I took a closer look. The failures are a result of #78879 and #77728 together that I merged around the same time. I should've run another pass of testing after merging #77728.

@kunalspathak
Copy link
Member Author

Ah, so #77728 code was also exercised for x64 after #78879. Makes sense.

@kunalspathak
Copy link
Member Author

Ah, so #77728 code was also exercised for x64 after #78879. Makes sense.

Yep. I reverted #77728 and it doesn't repro anymore. Do you remember if you trigggered jitstress after #77728 was merged? Probably not. If it was the case, then it should have picked those changes.

@jakobbotsch
Copy link
Member

jakobbotsch commented Dec 6, 2022

Do you remember if you trigggered jitstress after #77728 was merged? Probably not. If it was the case, then it should have picked those changes.

Right, I didn't, that's why I was saying I should have run another pass of testing after that.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failure JIT\\Performance\\CodeQuality\\Serialization\\Serialize\\Serialize.cmd
3 participants