Do not retype small locals in `FIELD_LIST`s on x86 #74030

SingleAccretion · 2022-08-16T21:23:51Z

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.

ghost · 2022-08-16T21:24:05Z

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:	-

SingleAccretion · 2022-08-18T15:49:11Z

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.

BruceForstall · 2022-08-24T18:24:00Z

@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 · 2022-08-24T18:32:13Z

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 · 2022-08-25T17:28:00Z

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

BruceForstall · 2022-09-02T22:54:53Z

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

BruceForstall · 2022-09-02T22:56:38Z

/backport to release/7.0

github-actions · 2022-09-02T22:56:49Z

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

ghost 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

JulieLeeMSFT added this to the 8.0.0 milestone Aug 16, 2022

JulieLeeMSFT assigned SingleAccretion Aug 16, 2022

SingleAccretion force-pushed the Fixing-Small-Type-Retyping branch from 1b6740b to 29c8bb8 Compare August 18, 2022 09:05

runfoapp bot mentioned this pull request Aug 18, 2022

Infra improvements for Helix #68176

Closed

SingleAccretion marked this pull request as ready for review August 18, 2022 15:49

SingleAccretion added 3 commits August 22, 2022 12:41

Add a test

4ceb34b

Do not widen uses of normalize-on-load locals

3dd54c0

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.

"Load/StoreWithPush" -> "Load/StoreFullSlot"

1a344dd

The load variant in codegen now has double purpose.

SingleAccretion force-pushed the Fixing-Small-Type-Retyping branch from 29c8bb8 to 1a344dd Compare August 22, 2022 09:41

BruceForstall modified the milestones: 8.0.0, 7.0.0 Aug 24, 2022

BruceForstall approved these changes Aug 24, 2022

View reviewed changes

SingleAccretion mentioned this pull request Aug 25, 2022

JIT: Assertion failed 'genTypeSize(fieldNode) == TARGET_POINTER_SIZE' during 'Generate code' #74562

Closed

JulieLeeMSFT approved these changes Aug 25, 2022

View reviewed changes

BruceForstall approved these changes Sep 2, 2022

View reviewed changes

github-actions bot mentioned this pull request Sep 2, 2022

[release/7.0] Do not retype small locals in FIELD_LISTs on x86 #75036

Merged

BruceForstall merged commit 2aed1fd into dotnet:main Sep 3, 2022

SingleAccretion deleted the Fixing-Small-Type-Retyping branch September 3, 2022 20:54

ghost locked as resolved and limited conversation to collaborators Oct 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Do not retype small locals in `FIELD_LIST`s on x86 #74030

Do not retype small locals in `FIELD_LIST`s on x86 #74030

Uh oh!

SingleAccretion commented Aug 16, 2022 •

edited

Loading

Uh oh!

ghost commented Aug 16, 2022

Uh oh!

SingleAccretion commented Aug 18, 2022

Uh oh!

BruceForstall commented Aug 24, 2022

Uh oh!

SingleAccretion commented Aug 24, 2022 •

edited

Loading

Uh oh!

JulieLeeMSFT commented Aug 25, 2022

Uh oh!

BruceForstall commented Sep 2, 2022

Uh oh!

BruceForstall commented Sep 2, 2022

Uh oh!

github-actions bot commented Sep 2, 2022

Uh oh!

Uh oh!

Do not retype small locals in FIELD_LISTs on x86 #74030

Do not retype small locals in FIELD_LISTs on x86 #74030

Uh oh!

Conversation

SingleAccretion commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 16, 2022

Uh oh!

SingleAccretion commented Aug 18, 2022

Uh oh!

BruceForstall commented Aug 24, 2022

Uh oh!

SingleAccretion commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JulieLeeMSFT commented Aug 25, 2022

Uh oh!

BruceForstall commented Sep 2, 2022

Uh oh!

BruceForstall commented Sep 2, 2022

Uh oh!

github-actions bot commented Sep 2, 2022

Uh oh!

Uh oh!

Do not retype small locals in `FIELD_LIST`s on x86 #74030

Do not retype small locals in `FIELD_LIST`s on x86 #74030

SingleAccretion commented Aug 16, 2022 •

edited

Loading

SingleAccretion commented Aug 24, 2022 •

edited

Loading