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

JIT: Use small register types for some enregisterable locals #67274

Merged
merged 2 commits into from
Mar 29, 2022
Merged

JIT: Use small register types for some enregisterable locals #67274

merged 2 commits into from
Mar 29, 2022

Conversation

AndyAyersMS
Copy link
Member

Fix two cases where small enregisterable locals can't be saved to the stack
using actual (widened) types:

  • small memory args for OSX ARM64
  • promoted fields of OSR locals

Closes #67152.
Closes #67188.

@AndyAyersMS
JIT: Use small register types for some enregisterable locals
b10d561 
Fix two cases where small enregisterable locals can't be saved to the stack
using actual (widened) types:
* small memory args for OSX ARM64
* promoted fields of OSR locals

Closes dotnet#67152.
Closes dotnet#67188.
@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 Mar 29, 2022
@ghost ghost assigned AndyAyersMS Mar 29, 2022
@ghost
Copy link

ghost commented Mar 29, 2022

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

Issue Details

Fix two cases where small enregisterable locals can't be saved to the stack
using actual (widened) types:

  • small memory args for OSX ARM64
  • promoted fields of OSR locals

Closes #67152.
Closes #67188.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@AndyAyersMS
Copy link
Member Author

@EgorBo PTAL
cc @dotnet/jit-contrib

I don't have a simple repro as the failures above are only seen in spilling.

@AndyAyersMS AndyAyersMS mentioned this pull request Mar 29, 2022
Open
72 tasks
@AndyAyersMS
Copy link
Member Author

CI hit two test failures

Ubuntu amd64

  Starting:    System.Text.Json.SourceGeneration.Roslyn3.11.Tests (parallel test collections = on, max threads = 2)
Process terminated. Assertion failed.
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.InitializePropCache() in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs:line 613
   at System.Text.Json.Serialization.Converters.JsonMetadataServicesConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value) in /_/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs:line 60

Alpine arm64

  Starting:    System.Collections.Concurrent.Tests (parallel test collections = on, max threads = 4)

Assert failure(PID 23 [0x00000017], Thread: 116 [0x0074]): m_alignpad == 0
    File: /__w/1/s/src/coreclr/vm/syncblk.cpp Line: 2952
    Image: /root/helix/work/correlation/dotnet

Neither repros locally (I don't have alpine arm64 hw, so ran on OSX).

Will retry these.

kunalspathak
kunalspathak approved these changes Mar 29, 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

SingleAccretion
SingleAccretion reviewed Mar 29, 2022
View reviewed changes
src/coreclr/jit/lclvars.cpp Outdated
if (varTypeIsSmall(TypeGet()))
{

#ifdef OSX_ARM64_ABI
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this define exist?

I would have expected a compMacOsArm64Abi() check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, thanks.

@jakobbotsch
Copy link
Member

We really should run Antigen and Fuzzlyn on macOS. I would expect this to be the kind of bug that they could find quickly.

@EgorBo
Copy link
Member

EgorBo commented Mar 29, 2022

We really should run Antigen and Fuzzlyn on macOS. I would expect this to be the kind of bug that they could find quickly.

is that something difficult to setup?

@jakobbotsch
Copy link
Member

We really should run Antigen and Fuzzlyn on macOS. I would expect this to be the kind of bug that they could find quickly.

is that something difficult to setup?

Probably not, I am making a PR right now :-)

SingleAccretion
SingleAccretion reviewed Mar 29, 2022
View reviewed changes
src/coreclr/jit/lclvars.cpp Outdated
Comment on lines 3844 to 3847
if (lvIsOSRLocal && lvIsStructField)
{
return GetRegisterType();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth adding a comment and/or assert about how this will not work on x86 due to the byteability requirement?

(I realize OSR isn't supported on x86 right now)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

@jakobbotsch jakobbotsch mentioned this pull request Mar 29, 2022
Merged
@AndyAyersMS
Copy link
Member Author

On the repro from #67188

;; before

        910003FD          mov     fp, sp
        B94013A2          ldr     w2, [fp,#16]	// [V08 arg8]
        B84123A1          ldr     w1, [fp,#18]	// [V09 arg9]
						;; bbWeight=1    PerfScore 5.50
G_M37948_IG02:              ;; offset=0010H
        13003C40          sxth    w0, w2
        13003C21          sxth    w1, w1
        0B010000          add     w0, w0, w1
        B90013A2          str     w2, [fp,#16]
        13003C4B          sxth    w11, w2
        0B0B0000          add     w0, w0, w11
        13003C01          sxth    w1, w0
        B80123A1          str     w1, [fp,#18]

;; after 

        79C023A2          ldrsh   w2, [fp,#16]	// [V08 arg8]
        79C027A1          ldrsh   w1, [fp,#18]	// [V09 arg9]
						;; bbWeight=1    PerfScore 5.50
G_M37948_IG02:              ;; offset=0010H
        13003C40          sxth    w0, w2
        13003C21          sxth    w1, w1
        0B010000          add     w0, w0, w1
        790023A2          strh    w2, [fp,#16]
        13003C4B          sxth    w11, w2
        0B0B0000          add     w0, w0, w11
        13003C01          sxth    w1, w0
        790027A1          strh    w1, [fp,#18]

@AndyAyersMS AndyAyersMS merged commit 7bf9ae2 into dotnet:main Mar 29, 2022
@AndyAyersMS AndyAyersMS mentioned this pull request Mar 30, 2022
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@SingleAccretion SingleAccretion SingleAccretion left review comments

@kunalspathak kunalspathak kunalspathak approved these changes

@EgorBo EgorBo EgorBo approved these changes

Assignees

@AndyAyersMS AndyAyersMS

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Spilling can overwrite adjacent stack parameters on ARM64 macOS ABI OSR failure in System.Text.Json.Tests
5 participants
@AndyAyersMS @jakobbotsch @EgorBo @kunalspathak @SingleAccretion