Skip to content

Do not retype small locals in FIELD_LISTs on x86 - #74030

Merged
BruceForstall merged 3 commits into
dotnet:mainfrom
SingleAccretion:Fixing-Small-Type-Retyping
Sep 3, 2022
Merged

Do not retype small locals in FIELD_LISTs on x86#74030
BruceForstall merged 3 commits into
dotnet:mainfrom
SingleAccretion:Fixing-Small-Type-Retyping

Conversation

@SingleAccretion

@SingleAccretionSingleAccretion commented Aug 16, 2022

Copy link
Copy Markdown
Contributor

This is not correct for normalize-on-load register candidates.

Fixes#73951.
Fixes#74562.

A tiny regression on x86, due to using extending moves in more cases.

@ghostghost added 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 labels Aug 16, 2022
@ghost

Copy link
Copy Markdown

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

Issue Details

This is not correct for normalize-on-load register candidates.

Fixes #73951.

Author:SingleAccretion
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@JulieLeeMSFTJulieLeeMSFT added this to the 8.0.0 milestone Aug 16, 2022
@SingleAccretion
SingleAccretionforce-pushed the Fixing-Small-Type-Retyping branch from 1b6740b to 29c8bb8CompareAugust 18, 2022 09:05
@runfoapprunfoappBot mentioned this pull request Aug 18, 2022
@SingleAccretion
SingleAccretion marked this pull request as ready for review August 18, 2022 15:49
@SingleAccretion

Copy link
Copy Markdown
ContributorAuthor

SPMI failures look pre-existing.

@dotnet/jit-contrib

This breaks the assumption that all LCL_VAR uses of such locals,
when typed small, are normalized (and equivalent to each other).
Instead, explicitly check for legality of widening the loads in LSRA and codegen.
The load variant in codegen now has double purpose.
@SingleAccretion
SingleAccretionforce-pushed the Fixing-Small-Type-Retyping branch from 29c8bb8 to 1a344ddCompareAugust 22, 2022 09:41
@BruceForstallBruceForstall modified the milestones: 8.0.0, 7.0.0Aug 24, 2022
@BruceForstall

Copy link
Copy Markdown
Contributor

@SingleAccretion When I run the new unit test locally, it passes even without your fix. I'm on 676a6bb

Also, I notice when building the test locally I get a warning:

Program.cs(11,28): warning CS8618: Non-nullable field 's_rt' must contain a non-null value when exiting constructor. Consider declaring the field as nullable.

I don't see this in the CI build, but maybe they suppress all warnings?

@SingleAccretion

SingleAccretion commented Aug 24, 2022

Copy link
Copy Markdown
ContributorAuthor

Hmm, that is very strange. I don't see any nullability warnings locally:

C:\Users\Accretion\source\dotnet\runtime\src\tests\JIT\Regression\JitBlue\Runtime_73951> dotnet build
MSBuild version 17.4.0-preview-22378-04+827c1bf9c for .NET
Determining projects to restore...
Restored C:\Users\Accretion\source\dotnet\runtime\src\tests\JIT\Regression\JitBlue\Runtime_73951\Runtime_73951.csproj (in 219 ms).
Restored C:\Users\Accretion\source\dotnet\runtime\src\tests\Common\XUnitWrapperGenerator\XUnitWrapperGenerator.csproj (in 235 ms).
C:\Users\Accretion\source\dotnet\runtime\artifacts\tests\coreclr\packages\JIT\Regression\JitBlue\Runtime_73951\Runtime_73951\Runtime_73951.csproj.nuget.g.props(17,5): warning MSB4011: "C
:\Users\Accretion\.nuget\packages\microsoft.net.illink.tasks\7.0.100-1.22415.4\build\Microsoft.NET.ILLink.Tasks.props" cannot be imported again. It was already imported at "C:\Users\Accr
etion\source\dotnet\runtime\artifacts\tests\coreclr\packages\Common\test_dependencies\test_dependencies\test_dependencies.csproj.nuget.g.props (16,5)". This is most likely a build author
ing error. This subsequent import will be ignored. [C:\Users\Accretion\source\dotnet\runtime\src\tests\JIT\Regression\JitBlue\Runtime_73951\Runtime_73951.csproj]
C:\Users\Accretion\source\dotnet\runtime\artifacts\tests\coreclr\packages\JIT\Regression\JitBlue\Runtime_73951\Runtime_73951\Runtime_73951.csproj.nuget.g.props(18,5): warning MSB4011: "C
:\Users\Accretion\.nuget\packages\microsoft.net.compilers.toolset\4.4.0-2.22412.11\build\Microsoft.Net.Compilers.Toolset.props" cannot be imported again. It was already imported at "C:\U
sers\Accretion\source\dotnet\runtime\artifacts\tests\coreclr\packages\Common\test_dependencies\test_dependencies\test_dependencies.csproj.nuget.g.props (17,5)". This is most likely a bui
ld authoring error. This subsequent import will be ignored. [C:\Users\Accretion\source\dotnet\runtime\src\tests\JIT\Regression\JitBlue\Runtime_73951\Runtime_73951.csproj]
XUnitWrapperGenerator -> C:\Users\Accretion\source\dotnet\runtime\artifacts\tests\coreclr\windows.x64.Debug\Common\XUnitWrapperGenerator\XUnitWrapperGenerator\XUnitWrapperGenerator.dll
Runtime_73951 -> C:\Users\Accretion\source\dotnet\runtime\artifacts\tests\coreclr\windows.x64.Debug\JIT\Regression\JitBlue\Runtime_73951\Runtime_73951\Runtime_73951.dll
Build succeeded.
C:\Users\Accretion\source\dotnet\runtime\artifacts\tests\coreclr\packages\JIT\Regression\JitBlue\Runtime_73951\Runtime_73951\Runtime_73951.csproj.nuget.g.props(17,5): warning MSB4011: "C
:\Users\Accretion\.nuget\packages\microsoft.net.illink.tasks\7.0.100-1.22415.4\build\Microsoft.NET.ILLink.Tasks.props" cannot be imported again. It was already imported at "C:\Users\Accr
etion\source\dotnet\runtime\artifacts\tests\coreclr\packages\Common\test_dependencies\test_dependencies\test_dependencies.csproj.nuget.g.props (16,5)". This is most likely a build author
ing error. This subsequent import will be ignored. [C:\Users\Accretion\source\dotnet\runtime\src\tests\JIT\Regression\JitBlue\Runtime_73951\Runtime_73951.csproj]
C:\Users\Accretion\source\dotnet\runtime\artifacts\tests\coreclr\packages\JIT\Regression\JitBlue\Runtime_73951\Runtime_73951\Runtime_73951.csproj.nuget.g.props(18,5): warning MSB4011: "C
:\Users\Accretion\.nuget\packages\microsoft.net.compilers.toolset\4.4.0-2.22412.11\build\Microsoft.Net.Compilers.Toolset.props" cannot be imported again. It was already imported at "C:\U
sers\Accretion\source\dotnet\runtime\artifacts\tests\coreclr\packages\Common\test_dependencies\test_dependencies\test_dependencies.csproj.nuget.g.props (17,5)". This is most likely a bui
ld authoring error. This subsequent import will be ignored. [C:\Users\Accretion\source\dotnet\runtime\src\tests\JIT\Regression\JitBlue\Runtime_73951\Runtime_73951.csproj]
2 Warning(s)
0 Error(s)
Time Elapsed 00:00:10.44

Certainly, I would expect our test built to globally disable nullability.

And the test does fail without and pass with the fix:

C:\Users\Accretion\source\dotnet\runtime\src\tests\JIT\Regression\JitBlue\Runtime_73951> set-test-env -a x86 -b
CORE_ROOT: C:\Users\Accretion\source\dotnet\runtime-base\artifacts\tests\coreclr\windows.x86.Checked\Tests\Core_Root
DOTNET_TieredCompilation: 0
# The "base" commit is '14a65ecaf86207b24d4aba6ff84806f87eacb993'
PS C:\Users\Accretion\source\dotnet\runtime\src\tests\JIT\Regression\JitBlue\Runtime_73951> C:\Users\Accretion\source\dotnet\runtime\artifacts\tests\coreclr\windows.x64.Debug\JIT\Regression\JitBlue\Runtime_73951\Runtime_73951\Runtime_73951.cmd
BEGIN EXECUTION
"C:\Users\Accretion\source\dotnet\runtime-base\artifacts\tests\coreclr\windows.x86.Checked\Tests\Core_Root\corerun.exe" Runtime_73951.dll
Expected: 100
Actual: 101
END EXECUTION - FAILED
FAILED
PS C:\Users\Accretion\source\dotnet\runtime\src\tests\JIT\Regression\JitBlue\Runtime_73951> set-test-env -a x86
CORE_ROOT: C:\Users\Accretion\source\dotnet\build\CustomCoreRoot_x86
DOTNET_TieredCompilation: 0
PS C:\Users\Accretion\source\dotnet\runtime\src\tests\JIT\Regression\JitBlue\Runtime_73951> C:\Users\Accretion\source\dotnet\runtime\artifacts\tests\coreclr\windows.x64.Debug\JIT\Regression\JitBlue\Runtime_73951\Runtime_73951\Runtime_73951.cmd
BEGIN EXECUTION
"C:\Users\Accretion\source\dotnet\build\CustomCoreRoot_x86\corerun.exe" Runtime_73951.dll
Expected: 100
Actual: 100
END EXECUTION - PASSED
PASSED

There is a detail in the test (the original Fuzzlyn code from which it was adapted also has this property) that to fail it needs to read some random bits from the stack, so perhaps it could be influenced by the environment...?

(But it does fail for me consistently, FWIW)

@JulieLeeMSFT

Copy link
Copy Markdown
Member

@BruceForstall please backport this to 7.0 once all tests pass.

@BruceForstall

Copy link
Copy Markdown
Contributor

I was able to (finally) reproduce the baseline failure and diff code non-failure.

@BruceForstall

Copy link
Copy Markdown
Contributor

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2982283022

@BruceForstall
BruceForstall merged commit 2aed1fd into dotnet:mainSep 3, 2022
@SingleAccretion
SingleAccretion deleted the Fixing-Small-Type-Retyping branch September 3, 2022 20:54
@ghostghost locked as resolved and limited conversation to collaborators Oct 4, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

3 participants

@SingleAccretion@BruceForstall@JulieLeeMSFT