JIT: small OSR locals must be normalize on load - #84000

Merged
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959
Mar 28, 2023
Merged

JIT: small OSR locals must be normalize on load#84000
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959

Conversation

@AndyAyersMS

@AndyAyersMSAndyAyersMS commented Mar 27, 2023

Copy link
Copy Markdown
Member

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0.

Fixes#83959.

When I changed the importation strategy for OSR in dotnet#83910 it
exposed a latent issue -- small OSR locals must normalized on load.
Fixesdotnet#83959.
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 27, 2023
@ghost

Copy link
Copy Markdown

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

Issue Details

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load.

Fixes #83959.

Author:AndyAyersMS
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

@jakobbotsch PTAL
cc @dotnet/jit-contrib

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Expecting a modest number of diffs for OSR methods.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

Running into various issues:

Assert failure(PID 24585 [0x00006009], Thread: 24602 [0x601a]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
;; osx
./RunTests.sh: line 168: 50069 Illegal instruction: 4 "$RUNTIME_PATH/dotnet" exec --runtimeconfig System.Runtime.Tests.runtimeconfig.json --depsfile System.Runtime.Tests.deps.json xunit.console.dll System.Runtime.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=AdditionalTimezoneChecks -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing $RSP_FILE
/private/tmp/helix/working/B38E099C/w/ACDA0999/e
----- end Mon Mar 27 20:14:10 EDT 2023 ----- exit code 132 ----------------------------------------------------------
exit code 132 means SIGILL Illegal Instruction. Core dumped. Likely codegen issue.
Assert failure(PID 12116 [0x00002f54], Thread: 4788 [0x12b4]): Assertion failed 'op1->gtEffectiveVal() == base' in 'System.RuntimeType:GetMethodBase(System.RuntimeType,int):System.Reflection.MethodBase' during 'Assertion prop' (IL size 480; hash 0xbca2468f; Tier1)
File: D:\a\_work\1\s\src\coreclr\jit\gentree.cpp Line: 4474

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

Seems like it does, but locally I am hitting another odd error. Running on a Coffee Lake i7 8700 so no AVX-512, I end up here when running under WSL2 when built against ed3721b.

cc @tannergooding

## JIT/opt/Compares/compares/compares.sh * thread #1, name = 'corerun', stop reason = signal SIGILL: illegal instruction operand
frame #0: 0x00007ffff776560d libcoreclr.so`RtlRestoreContext at context2.S:140
137 // See https://github.com/apple/darwin-xnu/blob/main/osfmk/i386/fpu.c#L174
138
139 // Restore the ZMM_Hi256 state
-> 140 vinsertf64x4 zmm0, zmm0, ymmword ptr [rdi + (CONTEXT_Zmm0H + 0 * 32)], 1
141 vinsertf64x4 zmm1, zmm1, ymmword ptr [rdi + (CONTEXT_Zmm0H + 1 * 32)], 1
142 vinsertf64x4 zmm2, zmm2, ymmword ptr [rdi + (CONTEXT_Zmm0H + 2 * 32)], 1
143 vinsertf64x4 zmm3, zmm3, ymmword ptr [rdi + (CONTEXT_Zmm0H + 3 * 32)], 1

@tannergooding

Copy link
Copy Markdown
Member

This would be caused by #83784

We're supposed to skip this bit if CONTEXT.XStateFeaturesMask hasn't been marked with XSTATE_MASK_AVX512: https://github.com/dotnet/runtime/pull/83784/files#diff-7fb8fda5dbdf85daa35acb2847f3613b2ca50e1dafafae5475e5443589258eacR128-R129

That correspondingly should only be set if FPREG_HasAvx512Registers returns true: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR964-R979

Which itself should only be getting set if the Linux kernel itself reports XFEATURE_MASK_AVX512 in the native thread context: https://github.com/dotnet/runtime/pull/83784/files#diff-6bd99da7ca73739fa65674c40dcb32ecc9af9bcaaff94fdc3d0f4e13704c2331R511-R525

and correspondingly the CPUID query also reports all 5 ISAs as supported: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR336-R383

Will need to dig into this a bit to try and repro things...

@tannergooding

Copy link
Copy Markdown
Member

Found the issue: #84012

Comment threadsrc/coreclr/jit/compiler.h Outdated
Comment on lines +1102 to +1109
(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);
}

bool lvNormalizeOnStore() const
{
return varTypeIsSmall(TypeGet()) &&
// lvIsStructField is treated the same as the aliased local, see fgDoNormalizeOnStore.
!(lvIsParam || m_addrExposed || lvIsStructField);
!(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It took me a while to guess that the reason is that the local could have been marked normalize-on-load in tier-0 due to the more conservative address exposure there. Maybe add a comment to that effect?

Also, I guess if we wanted to we could scope this down to just OSR locals that were address exposed in tier 0, since we have that information available.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about scoping it down, so let me do that (will need a new bit on lclvar dsc).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo, runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

So far just this one failure has recurred:

 Discovering: System.Runtime.Intrinsics.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Runtime.Intrinsics.Tests (found 1023 test cases)
Starting: System.Runtime.Intrinsics.Tests (parallel test collections = on, max threads = 2)
Assert failure(PID 23193 [0x00005a99], Thread: 23208 [0x5aa8]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
File: /__w/1/s/src/coreclr/jit/emitxarch.cpp Line: 7810
Image: /datadisks/disk1/work/B9AF0A02/p/dotnet

Does not repro locally, suspect it requires AVX-512.

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

set DOTNET_TieredCompilation=1
set DOTNET_ReadyToRun=0
set DOTNET_TC_QuickJitForLoops=1
set DOTNET_TieredPGO=1
set DOTNET_JitRandomGuardedDevirtualization=1
set DOTNET_JitRandomEdgeCounts=1
set DOTNET_JitRandomlyCollect64BitCounts=1
17:06:48.864 Running test: baseservices/threading/regressions/2164/foreground-shutdown/foreground-shutdown.cmd
Return code: 1
Raw output file: C:\h\w\BAA209F1\w\ACE309A2\uploads\regressions\2164\foreground-shutdown\output.txt
Raw output:
BEGIN EXECUTION
"C:\h\w\BAA209F1\p\corerun.exe" -p "System.Reflection.Metadata.MetadataUpdater.IsSupported=false" foreground-shutdown.dll Xunit.Sdk.EqualException: Assert.Equal() Failure
Expected: 100
Actual: 101
at Xunit.Assert.Equal[T](T expected, T actual, IEqualityComparer`1 comparer) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 96
at Xunit.Assert.Equal[T](T expected, T actual) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 63
at __GeneratedMainWrapper.Main()
Expected: 100
Actual: 101
END EXECUTION - FAILED

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

Bruce pointed out a mismatched VM altjit would work -- I tried this via windows crossjit on linux -- and while I do indeed see zmm's in the disasm, I can't yet repro the assertion failure.

; Assembly listing for method System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this
; Emitting BLENDED_CODE for X64 CPU with AVX512 - Windows
; Tier-1 compilation
; OSR variant for entry point 0x11f
...
4889842420010000 mov qword ptr [rsp+120H], rax
62F17C4810B42470030000 vmovups zmm6, zmmword ptr[rsp+370H]
8BB4246C030000 mov esi, dword ptr [rsp+36CH]
...

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

There aren't any OSR methods in this test, so the failure seems unrelated.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Going to merge since we have potential OSR silent bad code; will try and repro the AVX512 issue some other way.

@AndyAyersMS
AndyAyersMS merged commit fd157a5 into dotnet:mainMar 28, 2023
@AndyAyersMSAndyAyersMS mentioned this pull request Mar 30, 2023
72 tasks
@ghostghost locked as resolved and limited conversation to collaborators Apr 28, 2023
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 SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[jitosr_stress_random] Failures in System.Text related tests

3 participants

@AndyAyersMS@tannergooding@jakobbotsch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

JIT: small OSR locals must be normalize on load - #84000

Merged
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959
Mar 28, 2023
Merged

JIT: small OSR locals must be normalize on load#84000
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959

Conversation

@AndyAyersMS

@AndyAyersMSAndyAyersMS commented Mar 27, 2023

Copy link
Copy Markdown
Member

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0.

Fixes#83959.

When I changed the importation strategy for OSR in dotnet#83910 it
exposed a latent issue -- small OSR locals must normalized on load.
Fixesdotnet#83959.
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 27, 2023
@ghost

Copy link
Copy Markdown

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

Issue Details

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load.

Fixes #83959.

Author:AndyAyersMS
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

@jakobbotsch PTAL
cc @dotnet/jit-contrib

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Expecting a modest number of diffs for OSR methods.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

Running into various issues:

Assert failure(PID 24585 [0x00006009], Thread: 24602 [0x601a]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
;; osx
./RunTests.sh: line 168: 50069 Illegal instruction: 4 "$RUNTIME_PATH/dotnet" exec --runtimeconfig System.Runtime.Tests.runtimeconfig.json --depsfile System.Runtime.Tests.deps.json xunit.console.dll System.Runtime.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=AdditionalTimezoneChecks -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing $RSP_FILE
/private/tmp/helix/working/B38E099C/w/ACDA0999/e
----- end Mon Mar 27 20:14:10 EDT 2023 ----- exit code 132 ----------------------------------------------------------
exit code 132 means SIGILL Illegal Instruction. Core dumped. Likely codegen issue.
Assert failure(PID 12116 [0x00002f54], Thread: 4788 [0x12b4]): Assertion failed 'op1->gtEffectiveVal() == base' in 'System.RuntimeType:GetMethodBase(System.RuntimeType,int):System.Reflection.MethodBase' during 'Assertion prop' (IL size 480; hash 0xbca2468f; Tier1)
File: D:\a\_work\1\s\src\coreclr\jit\gentree.cpp Line: 4474

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

Seems like it does, but locally I am hitting another odd error. Running on a Coffee Lake i7 8700 so no AVX-512, I end up here when running under WSL2 when built against ed3721b.

cc @tannergooding

## JIT/opt/Compares/compares/compares.sh * thread #1, name = 'corerun', stop reason = signal SIGILL: illegal instruction operand
frame #0: 0x00007ffff776560d libcoreclr.so`RtlRestoreContext at context2.S:140
137 // See https://github.com/apple/darwin-xnu/blob/main/osfmk/i386/fpu.c#L174
138
139 // Restore the ZMM_Hi256 state
-> 140 vinsertf64x4 zmm0, zmm0, ymmword ptr [rdi + (CONTEXT_Zmm0H + 0 * 32)], 1
141 vinsertf64x4 zmm1, zmm1, ymmword ptr [rdi + (CONTEXT_Zmm0H + 1 * 32)], 1
142 vinsertf64x4 zmm2, zmm2, ymmword ptr [rdi + (CONTEXT_Zmm0H + 2 * 32)], 1
143 vinsertf64x4 zmm3, zmm3, ymmword ptr [rdi + (CONTEXT_Zmm0H + 3 * 32)], 1

@tannergooding

Copy link
Copy Markdown
Member

This would be caused by #83784

We're supposed to skip this bit if CONTEXT.XStateFeaturesMask hasn't been marked with XSTATE_MASK_AVX512: https://github.com/dotnet/runtime/pull/83784/files#diff-7fb8fda5dbdf85daa35acb2847f3613b2ca50e1dafafae5475e5443589258eacR128-R129

That correspondingly should only be set if FPREG_HasAvx512Registers returns true: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR964-R979

Which itself should only be getting set if the Linux kernel itself reports XFEATURE_MASK_AVX512 in the native thread context: https://github.com/dotnet/runtime/pull/83784/files#diff-6bd99da7ca73739fa65674c40dcb32ecc9af9bcaaff94fdc3d0f4e13704c2331R511-R525

and correspondingly the CPUID query also reports all 5 ISAs as supported: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR336-R383

Will need to dig into this a bit to try and repro things...

@tannergooding

Copy link
Copy Markdown
Member

Found the issue: #84012

Comment threadsrc/coreclr/jit/compiler.h Outdated
Comment on lines +1102 to +1109
(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);
}

bool lvNormalizeOnStore() const
{
return varTypeIsSmall(TypeGet()) &&
// lvIsStructField is treated the same as the aliased local, see fgDoNormalizeOnStore.
!(lvIsParam || m_addrExposed || lvIsStructField);
!(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It took me a while to guess that the reason is that the local could have been marked normalize-on-load in tier-0 due to the more conservative address exposure there. Maybe add a comment to that effect?

Also, I guess if we wanted to we could scope this down to just OSR locals that were address exposed in tier 0, since we have that information available.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about scoping it down, so let me do that (will need a new bit on lclvar dsc).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo, runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

So far just this one failure has recurred:

 Discovering: System.Runtime.Intrinsics.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Runtime.Intrinsics.Tests (found 1023 test cases)
Starting: System.Runtime.Intrinsics.Tests (parallel test collections = on, max threads = 2)
Assert failure(PID 23193 [0x00005a99], Thread: 23208 [0x5aa8]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
File: /__w/1/s/src/coreclr/jit/emitxarch.cpp Line: 7810
Image: /datadisks/disk1/work/B9AF0A02/p/dotnet

Does not repro locally, suspect it requires AVX-512.

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

set DOTNET_TieredCompilation=1
set DOTNET_ReadyToRun=0
set DOTNET_TC_QuickJitForLoops=1
set DOTNET_TieredPGO=1
set DOTNET_JitRandomGuardedDevirtualization=1
set DOTNET_JitRandomEdgeCounts=1
set DOTNET_JitRandomlyCollect64BitCounts=1
17:06:48.864 Running test: baseservices/threading/regressions/2164/foreground-shutdown/foreground-shutdown.cmd
Return code: 1
Raw output file: C:\h\w\BAA209F1\w\ACE309A2\uploads\regressions\2164\foreground-shutdown\output.txt
Raw output:
BEGIN EXECUTION
"C:\h\w\BAA209F1\p\corerun.exe" -p "System.Reflection.Metadata.MetadataUpdater.IsSupported=false" foreground-shutdown.dll Xunit.Sdk.EqualException: Assert.Equal() Failure
Expected: 100
Actual: 101
at Xunit.Assert.Equal[T](T expected, T actual, IEqualityComparer`1 comparer) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 96
at Xunit.Assert.Equal[T](T expected, T actual) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 63
at __GeneratedMainWrapper.Main()
Expected: 100
Actual: 101
END EXECUTION - FAILED

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

Bruce pointed out a mismatched VM altjit would work -- I tried this via windows crossjit on linux -- and while I do indeed see zmm's in the disasm, I can't yet repro the assertion failure.

; Assembly listing for method System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this
; Emitting BLENDED_CODE for X64 CPU with AVX512 - Windows
; Tier-1 compilation
; OSR variant for entry point 0x11f
...
4889842420010000 mov qword ptr [rsp+120H], rax
62F17C4810B42470030000 vmovups zmm6, zmmword ptr[rsp+370H]
8BB4246C030000 mov esi, dword ptr [rsp+36CH]
...

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

There aren't any OSR methods in this test, so the failure seems unrelated.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Going to merge since we have potential OSR silent bad code; will try and repro the AVX512 issue some other way.

@AndyAyersMS
AndyAyersMS merged commit fd157a5 into dotnet:mainMar 28, 2023
@AndyAyersMSAndyAyersMS mentioned this pull request Mar 30, 2023
72 tasks
@ghostghost locked as resolved and limited conversation to collaborators Apr 28, 2023
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 SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[jitosr_stress_random] Failures in System.Text related tests

3 participants

@AndyAyersMS@tannergooding@jakobbotsch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

JIT: small OSR locals must be normalize on load - #84000

Merged
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959
Mar 28, 2023
Merged

JIT: small OSR locals must be normalize on load#84000
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959

Conversation

@AndyAyersMS

@AndyAyersMSAndyAyersMS commented Mar 27, 2023

Copy link
Copy Markdown
Member

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0.

Fixes#83959.

When I changed the importation strategy for OSR in dotnet#83910 it
exposed a latent issue -- small OSR locals must normalized on load.
Fixesdotnet#83959.
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 27, 2023
@ghost

Copy link
Copy Markdown

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

Issue Details

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load.

Fixes #83959.

Author:AndyAyersMS
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

@jakobbotsch PTAL
cc @dotnet/jit-contrib

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Expecting a modest number of diffs for OSR methods.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

Running into various issues:

Assert failure(PID 24585 [0x00006009], Thread: 24602 [0x601a]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
;; osx
./RunTests.sh: line 168: 50069 Illegal instruction: 4 "$RUNTIME_PATH/dotnet" exec --runtimeconfig System.Runtime.Tests.runtimeconfig.json --depsfile System.Runtime.Tests.deps.json xunit.console.dll System.Runtime.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=AdditionalTimezoneChecks -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing $RSP_FILE
/private/tmp/helix/working/B38E099C/w/ACDA0999/e
----- end Mon Mar 27 20:14:10 EDT 2023 ----- exit code 132 ----------------------------------------------------------
exit code 132 means SIGILL Illegal Instruction. Core dumped. Likely codegen issue.
Assert failure(PID 12116 [0x00002f54], Thread: 4788 [0x12b4]): Assertion failed 'op1->gtEffectiveVal() == base' in 'System.RuntimeType:GetMethodBase(System.RuntimeType,int):System.Reflection.MethodBase' during 'Assertion prop' (IL size 480; hash 0xbca2468f; Tier1)
File: D:\a\_work\1\s\src\coreclr\jit\gentree.cpp Line: 4474

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

Seems like it does, but locally I am hitting another odd error. Running on a Coffee Lake i7 8700 so no AVX-512, I end up here when running under WSL2 when built against ed3721b.

cc @tannergooding

## JIT/opt/Compares/compares/compares.sh * thread #1, name = 'corerun', stop reason = signal SIGILL: illegal instruction operand
frame #0: 0x00007ffff776560d libcoreclr.so`RtlRestoreContext at context2.S:140
137 // See https://github.com/apple/darwin-xnu/blob/main/osfmk/i386/fpu.c#L174
138
139 // Restore the ZMM_Hi256 state
-> 140 vinsertf64x4 zmm0, zmm0, ymmword ptr [rdi + (CONTEXT_Zmm0H + 0 * 32)], 1
141 vinsertf64x4 zmm1, zmm1, ymmword ptr [rdi + (CONTEXT_Zmm0H + 1 * 32)], 1
142 vinsertf64x4 zmm2, zmm2, ymmword ptr [rdi + (CONTEXT_Zmm0H + 2 * 32)], 1
143 vinsertf64x4 zmm3, zmm3, ymmword ptr [rdi + (CONTEXT_Zmm0H + 3 * 32)], 1

@tannergooding

Copy link
Copy Markdown
Member

This would be caused by #83784

We're supposed to skip this bit if CONTEXT.XStateFeaturesMask hasn't been marked with XSTATE_MASK_AVX512: https://github.com/dotnet/runtime/pull/83784/files#diff-7fb8fda5dbdf85daa35acb2847f3613b2ca50e1dafafae5475e5443589258eacR128-R129

That correspondingly should only be set if FPREG_HasAvx512Registers returns true: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR964-R979

Which itself should only be getting set if the Linux kernel itself reports XFEATURE_MASK_AVX512 in the native thread context: https://github.com/dotnet/runtime/pull/83784/files#diff-6bd99da7ca73739fa65674c40dcb32ecc9af9bcaaff94fdc3d0f4e13704c2331R511-R525

and correspondingly the CPUID query also reports all 5 ISAs as supported: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR336-R383

Will need to dig into this a bit to try and repro things...

@tannergooding

Copy link
Copy Markdown
Member

Found the issue: #84012

Comment threadsrc/coreclr/jit/compiler.h Outdated
Comment on lines +1102 to +1109
(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);
}

bool lvNormalizeOnStore() const
{
return varTypeIsSmall(TypeGet()) &&
// lvIsStructField is treated the same as the aliased local, see fgDoNormalizeOnStore.
!(lvIsParam || m_addrExposed || lvIsStructField);
!(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It took me a while to guess that the reason is that the local could have been marked normalize-on-load in tier-0 due to the more conservative address exposure there. Maybe add a comment to that effect?

Also, I guess if we wanted to we could scope this down to just OSR locals that were address exposed in tier 0, since we have that information available.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about scoping it down, so let me do that (will need a new bit on lclvar dsc).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo, runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

So far just this one failure has recurred:

 Discovering: System.Runtime.Intrinsics.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Runtime.Intrinsics.Tests (found 1023 test cases)
Starting: System.Runtime.Intrinsics.Tests (parallel test collections = on, max threads = 2)
Assert failure(PID 23193 [0x00005a99], Thread: 23208 [0x5aa8]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
File: /__w/1/s/src/coreclr/jit/emitxarch.cpp Line: 7810
Image: /datadisks/disk1/work/B9AF0A02/p/dotnet

Does not repro locally, suspect it requires AVX-512.

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

set DOTNET_TieredCompilation=1
set DOTNET_ReadyToRun=0
set DOTNET_TC_QuickJitForLoops=1
set DOTNET_TieredPGO=1
set DOTNET_JitRandomGuardedDevirtualization=1
set DOTNET_JitRandomEdgeCounts=1
set DOTNET_JitRandomlyCollect64BitCounts=1
17:06:48.864 Running test: baseservices/threading/regressions/2164/foreground-shutdown/foreground-shutdown.cmd
Return code: 1
Raw output file: C:\h\w\BAA209F1\w\ACE309A2\uploads\regressions\2164\foreground-shutdown\output.txt
Raw output:
BEGIN EXECUTION
"C:\h\w\BAA209F1\p\corerun.exe" -p "System.Reflection.Metadata.MetadataUpdater.IsSupported=false" foreground-shutdown.dll Xunit.Sdk.EqualException: Assert.Equal() Failure
Expected: 100
Actual: 101
at Xunit.Assert.Equal[T](T expected, T actual, IEqualityComparer`1 comparer) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 96
at Xunit.Assert.Equal[T](T expected, T actual) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 63
at __GeneratedMainWrapper.Main()
Expected: 100
Actual: 101
END EXECUTION - FAILED

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

Bruce pointed out a mismatched VM altjit would work -- I tried this via windows crossjit on linux -- and while I do indeed see zmm's in the disasm, I can't yet repro the assertion failure.

; Assembly listing for method System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this
; Emitting BLENDED_CODE for X64 CPU with AVX512 - Windows
; Tier-1 compilation
; OSR variant for entry point 0x11f
...
4889842420010000 mov qword ptr [rsp+120H], rax
62F17C4810B42470030000 vmovups zmm6, zmmword ptr[rsp+370H]
8BB4246C030000 mov esi, dword ptr [rsp+36CH]
...

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

There aren't any OSR methods in this test, so the failure seems unrelated.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Going to merge since we have potential OSR silent bad code; will try and repro the AVX512 issue some other way.

@AndyAyersMS
AndyAyersMS merged commit fd157a5 into dotnet:mainMar 28, 2023
@AndyAyersMSAndyAyersMS mentioned this pull request Mar 30, 2023
72 tasks
@ghostghost locked as resolved and limited conversation to collaborators Apr 28, 2023
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 SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[jitosr_stress_random] Failures in System.Text related tests

3 participants

@AndyAyersMS@tannergooding@jakobbotsch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

JIT: small OSR locals must be normalize on load - #84000

Merged
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959
Mar 28, 2023
Merged

JIT: small OSR locals must be normalize on load#84000
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959

Conversation

@AndyAyersMS

@AndyAyersMSAndyAyersMS commented Mar 27, 2023

Copy link
Copy Markdown
Member

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0.

Fixes#83959.

When I changed the importation strategy for OSR in dotnet#83910 it
exposed a latent issue -- small OSR locals must normalized on load.
Fixesdotnet#83959.
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 27, 2023
@ghost

Copy link
Copy Markdown

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

Issue Details

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load.

Fixes #83959.

Author:AndyAyersMS
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

@jakobbotsch PTAL
cc @dotnet/jit-contrib

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Expecting a modest number of diffs for OSR methods.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

Running into various issues:

Assert failure(PID 24585 [0x00006009], Thread: 24602 [0x601a]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
;; osx
./RunTests.sh: line 168: 50069 Illegal instruction: 4 "$RUNTIME_PATH/dotnet" exec --runtimeconfig System.Runtime.Tests.runtimeconfig.json --depsfile System.Runtime.Tests.deps.json xunit.console.dll System.Runtime.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=AdditionalTimezoneChecks -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing $RSP_FILE
/private/tmp/helix/working/B38E099C/w/ACDA0999/e
----- end Mon Mar 27 20:14:10 EDT 2023 ----- exit code 132 ----------------------------------------------------------
exit code 132 means SIGILL Illegal Instruction. Core dumped. Likely codegen issue.
Assert failure(PID 12116 [0x00002f54], Thread: 4788 [0x12b4]): Assertion failed 'op1->gtEffectiveVal() == base' in 'System.RuntimeType:GetMethodBase(System.RuntimeType,int):System.Reflection.MethodBase' during 'Assertion prop' (IL size 480; hash 0xbca2468f; Tier1)
File: D:\a\_work\1\s\src\coreclr\jit\gentree.cpp Line: 4474

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

Seems like it does, but locally I am hitting another odd error. Running on a Coffee Lake i7 8700 so no AVX-512, I end up here when running under WSL2 when built against ed3721b.

cc @tannergooding

## JIT/opt/Compares/compares/compares.sh * thread #1, name = 'corerun', stop reason = signal SIGILL: illegal instruction operand
frame #0: 0x00007ffff776560d libcoreclr.so`RtlRestoreContext at context2.S:140
137 // See https://github.com/apple/darwin-xnu/blob/main/osfmk/i386/fpu.c#L174
138
139 // Restore the ZMM_Hi256 state
-> 140 vinsertf64x4 zmm0, zmm0, ymmword ptr [rdi + (CONTEXT_Zmm0H + 0 * 32)], 1
141 vinsertf64x4 zmm1, zmm1, ymmword ptr [rdi + (CONTEXT_Zmm0H + 1 * 32)], 1
142 vinsertf64x4 zmm2, zmm2, ymmword ptr [rdi + (CONTEXT_Zmm0H + 2 * 32)], 1
143 vinsertf64x4 zmm3, zmm3, ymmword ptr [rdi + (CONTEXT_Zmm0H + 3 * 32)], 1

@tannergooding

Copy link
Copy Markdown
Member

This would be caused by #83784

We're supposed to skip this bit if CONTEXT.XStateFeaturesMask hasn't been marked with XSTATE_MASK_AVX512: https://github.com/dotnet/runtime/pull/83784/files#diff-7fb8fda5dbdf85daa35acb2847f3613b2ca50e1dafafae5475e5443589258eacR128-R129

That correspondingly should only be set if FPREG_HasAvx512Registers returns true: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR964-R979

Which itself should only be getting set if the Linux kernel itself reports XFEATURE_MASK_AVX512 in the native thread context: https://github.com/dotnet/runtime/pull/83784/files#diff-6bd99da7ca73739fa65674c40dcb32ecc9af9bcaaff94fdc3d0f4e13704c2331R511-R525

and correspondingly the CPUID query also reports all 5 ISAs as supported: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR336-R383

Will need to dig into this a bit to try and repro things...

@tannergooding

Copy link
Copy Markdown
Member

Found the issue: #84012

Comment threadsrc/coreclr/jit/compiler.h Outdated
Comment on lines +1102 to +1109
(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);
}

bool lvNormalizeOnStore() const
{
return varTypeIsSmall(TypeGet()) &&
// lvIsStructField is treated the same as the aliased local, see fgDoNormalizeOnStore.
!(lvIsParam || m_addrExposed || lvIsStructField);
!(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It took me a while to guess that the reason is that the local could have been marked normalize-on-load in tier-0 due to the more conservative address exposure there. Maybe add a comment to that effect?

Also, I guess if we wanted to we could scope this down to just OSR locals that were address exposed in tier 0, since we have that information available.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about scoping it down, so let me do that (will need a new bit on lclvar dsc).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo, runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

So far just this one failure has recurred:

 Discovering: System.Runtime.Intrinsics.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Runtime.Intrinsics.Tests (found 1023 test cases)
Starting: System.Runtime.Intrinsics.Tests (parallel test collections = on, max threads = 2)
Assert failure(PID 23193 [0x00005a99], Thread: 23208 [0x5aa8]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
File: /__w/1/s/src/coreclr/jit/emitxarch.cpp Line: 7810
Image: /datadisks/disk1/work/B9AF0A02/p/dotnet

Does not repro locally, suspect it requires AVX-512.

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

set DOTNET_TieredCompilation=1
set DOTNET_ReadyToRun=0
set DOTNET_TC_QuickJitForLoops=1
set DOTNET_TieredPGO=1
set DOTNET_JitRandomGuardedDevirtualization=1
set DOTNET_JitRandomEdgeCounts=1
set DOTNET_JitRandomlyCollect64BitCounts=1
17:06:48.864 Running test: baseservices/threading/regressions/2164/foreground-shutdown/foreground-shutdown.cmd
Return code: 1
Raw output file: C:\h\w\BAA209F1\w\ACE309A2\uploads\regressions\2164\foreground-shutdown\output.txt
Raw output:
BEGIN EXECUTION
"C:\h\w\BAA209F1\p\corerun.exe" -p "System.Reflection.Metadata.MetadataUpdater.IsSupported=false" foreground-shutdown.dll Xunit.Sdk.EqualException: Assert.Equal() Failure
Expected: 100
Actual: 101
at Xunit.Assert.Equal[T](T expected, T actual, IEqualityComparer`1 comparer) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 96
at Xunit.Assert.Equal[T](T expected, T actual) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 63
at __GeneratedMainWrapper.Main()
Expected: 100
Actual: 101
END EXECUTION - FAILED

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

Bruce pointed out a mismatched VM altjit would work -- I tried this via windows crossjit on linux -- and while I do indeed see zmm's in the disasm, I can't yet repro the assertion failure.

; Assembly listing for method System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this
; Emitting BLENDED_CODE for X64 CPU with AVX512 - Windows
; Tier-1 compilation
; OSR variant for entry point 0x11f
...
4889842420010000 mov qword ptr [rsp+120H], rax
62F17C4810B42470030000 vmovups zmm6, zmmword ptr[rsp+370H]
8BB4246C030000 mov esi, dword ptr [rsp+36CH]
...

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

There aren't any OSR methods in this test, so the failure seems unrelated.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Going to merge since we have potential OSR silent bad code; will try and repro the AVX512 issue some other way.

@AndyAyersMS
AndyAyersMS merged commit fd157a5 into dotnet:mainMar 28, 2023
@AndyAyersMSAndyAyersMS mentioned this pull request Mar 30, 2023
72 tasks
@ghostghost locked as resolved and limited conversation to collaborators Apr 28, 2023
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 SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[jitosr_stress_random] Failures in System.Text related tests

3 participants

@AndyAyersMS@tannergooding@jakobbotsch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

JIT: small OSR locals must be normalize on load - #84000

Merged
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959
Mar 28, 2023
Merged

JIT: small OSR locals must be normalize on load#84000
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959

Conversation

@AndyAyersMS

@AndyAyersMSAndyAyersMS commented Mar 27, 2023

Copy link
Copy Markdown
Member

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0.

Fixes#83959.

When I changed the importation strategy for OSR in dotnet#83910 it
exposed a latent issue -- small OSR locals must normalized on load.
Fixesdotnet#83959.
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 27, 2023
@ghost

Copy link
Copy Markdown

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

Issue Details

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load.

Fixes #83959.

Author:AndyAyersMS
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

@jakobbotsch PTAL
cc @dotnet/jit-contrib

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Expecting a modest number of diffs for OSR methods.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

Running into various issues:

Assert failure(PID 24585 [0x00006009], Thread: 24602 [0x601a]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
;; osx
./RunTests.sh: line 168: 50069 Illegal instruction: 4 "$RUNTIME_PATH/dotnet" exec --runtimeconfig System.Runtime.Tests.runtimeconfig.json --depsfile System.Runtime.Tests.deps.json xunit.console.dll System.Runtime.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=AdditionalTimezoneChecks -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing $RSP_FILE
/private/tmp/helix/working/B38E099C/w/ACDA0999/e
----- end Mon Mar 27 20:14:10 EDT 2023 ----- exit code 132 ----------------------------------------------------------
exit code 132 means SIGILL Illegal Instruction. Core dumped. Likely codegen issue.
Assert failure(PID 12116 [0x00002f54], Thread: 4788 [0x12b4]): Assertion failed 'op1->gtEffectiveVal() == base' in 'System.RuntimeType:GetMethodBase(System.RuntimeType,int):System.Reflection.MethodBase' during 'Assertion prop' (IL size 480; hash 0xbca2468f; Tier1)
File: D:\a\_work\1\s\src\coreclr\jit\gentree.cpp Line: 4474

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

Seems like it does, but locally I am hitting another odd error. Running on a Coffee Lake i7 8700 so no AVX-512, I end up here when running under WSL2 when built against ed3721b.

cc @tannergooding

## JIT/opt/Compares/compares/compares.sh * thread #1, name = 'corerun', stop reason = signal SIGILL: illegal instruction operand
frame #0: 0x00007ffff776560d libcoreclr.so`RtlRestoreContext at context2.S:140
137 // See https://github.com/apple/darwin-xnu/blob/main/osfmk/i386/fpu.c#L174
138
139 // Restore the ZMM_Hi256 state
-> 140 vinsertf64x4 zmm0, zmm0, ymmword ptr [rdi + (CONTEXT_Zmm0H + 0 * 32)], 1
141 vinsertf64x4 zmm1, zmm1, ymmword ptr [rdi + (CONTEXT_Zmm0H + 1 * 32)], 1
142 vinsertf64x4 zmm2, zmm2, ymmword ptr [rdi + (CONTEXT_Zmm0H + 2 * 32)], 1
143 vinsertf64x4 zmm3, zmm3, ymmword ptr [rdi + (CONTEXT_Zmm0H + 3 * 32)], 1

@tannergooding

Copy link
Copy Markdown
Member

This would be caused by #83784

We're supposed to skip this bit if CONTEXT.XStateFeaturesMask hasn't been marked with XSTATE_MASK_AVX512: https://github.com/dotnet/runtime/pull/83784/files#diff-7fb8fda5dbdf85daa35acb2847f3613b2ca50e1dafafae5475e5443589258eacR128-R129

That correspondingly should only be set if FPREG_HasAvx512Registers returns true: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR964-R979

Which itself should only be getting set if the Linux kernel itself reports XFEATURE_MASK_AVX512 in the native thread context: https://github.com/dotnet/runtime/pull/83784/files#diff-6bd99da7ca73739fa65674c40dcb32ecc9af9bcaaff94fdc3d0f4e13704c2331R511-R525

and correspondingly the CPUID query also reports all 5 ISAs as supported: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR336-R383

Will need to dig into this a bit to try and repro things...

@tannergooding

Copy link
Copy Markdown
Member

Found the issue: #84012

Comment threadsrc/coreclr/jit/compiler.h Outdated
Comment on lines +1102 to +1109
(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);
}

bool lvNormalizeOnStore() const
{
return varTypeIsSmall(TypeGet()) &&
// lvIsStructField is treated the same as the aliased local, see fgDoNormalizeOnStore.
!(lvIsParam || m_addrExposed || lvIsStructField);
!(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It took me a while to guess that the reason is that the local could have been marked normalize-on-load in tier-0 due to the more conservative address exposure there. Maybe add a comment to that effect?

Also, I guess if we wanted to we could scope this down to just OSR locals that were address exposed in tier 0, since we have that information available.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about scoping it down, so let me do that (will need a new bit on lclvar dsc).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo, runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

So far just this one failure has recurred:

 Discovering: System.Runtime.Intrinsics.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Runtime.Intrinsics.Tests (found 1023 test cases)
Starting: System.Runtime.Intrinsics.Tests (parallel test collections = on, max threads = 2)
Assert failure(PID 23193 [0x00005a99], Thread: 23208 [0x5aa8]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
File: /__w/1/s/src/coreclr/jit/emitxarch.cpp Line: 7810
Image: /datadisks/disk1/work/B9AF0A02/p/dotnet

Does not repro locally, suspect it requires AVX-512.

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

set DOTNET_TieredCompilation=1
set DOTNET_ReadyToRun=0
set DOTNET_TC_QuickJitForLoops=1
set DOTNET_TieredPGO=1
set DOTNET_JitRandomGuardedDevirtualization=1
set DOTNET_JitRandomEdgeCounts=1
set DOTNET_JitRandomlyCollect64BitCounts=1
17:06:48.864 Running test: baseservices/threading/regressions/2164/foreground-shutdown/foreground-shutdown.cmd
Return code: 1
Raw output file: C:\h\w\BAA209F1\w\ACE309A2\uploads\regressions\2164\foreground-shutdown\output.txt
Raw output:
BEGIN EXECUTION
"C:\h\w\BAA209F1\p\corerun.exe" -p "System.Reflection.Metadata.MetadataUpdater.IsSupported=false" foreground-shutdown.dll Xunit.Sdk.EqualException: Assert.Equal() Failure
Expected: 100
Actual: 101
at Xunit.Assert.Equal[T](T expected, T actual, IEqualityComparer`1 comparer) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 96
at Xunit.Assert.Equal[T](T expected, T actual) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 63
at __GeneratedMainWrapper.Main()
Expected: 100
Actual: 101
END EXECUTION - FAILED

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

Bruce pointed out a mismatched VM altjit would work -- I tried this via windows crossjit on linux -- and while I do indeed see zmm's in the disasm, I can't yet repro the assertion failure.

; Assembly listing for method System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this
; Emitting BLENDED_CODE for X64 CPU with AVX512 - Windows
; Tier-1 compilation
; OSR variant for entry point 0x11f
...
4889842420010000 mov qword ptr [rsp+120H], rax
62F17C4810B42470030000 vmovups zmm6, zmmword ptr[rsp+370H]
8BB4246C030000 mov esi, dword ptr [rsp+36CH]
...

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

There aren't any OSR methods in this test, so the failure seems unrelated.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Going to merge since we have potential OSR silent bad code; will try and repro the AVX512 issue some other way.

@AndyAyersMS
AndyAyersMS merged commit fd157a5 into dotnet:mainMar 28, 2023
@AndyAyersMSAndyAyersMS mentioned this pull request Mar 30, 2023
72 tasks
@ghostghost locked as resolved and limited conversation to collaborators Apr 28, 2023
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 SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[jitosr_stress_random] Failures in System.Text related tests

3 participants

@AndyAyersMS@tannergooding@jakobbotsch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

JIT: small OSR locals must be normalize on load - #84000

Merged
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959
Mar 28, 2023
Merged

JIT: small OSR locals must be normalize on load#84000
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959

Conversation

@AndyAyersMS

@AndyAyersMSAndyAyersMS commented Mar 27, 2023

Copy link
Copy Markdown
Member

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0.

Fixes#83959.

When I changed the importation strategy for OSR in dotnet#83910 it
exposed a latent issue -- small OSR locals must normalized on load.
Fixesdotnet#83959.
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 27, 2023
@ghost

Copy link
Copy Markdown

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

Issue Details

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load.

Fixes #83959.

Author:AndyAyersMS
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

@jakobbotsch PTAL
cc @dotnet/jit-contrib

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Expecting a modest number of diffs for OSR methods.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

Running into various issues:

Assert failure(PID 24585 [0x00006009], Thread: 24602 [0x601a]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
;; osx
./RunTests.sh: line 168: 50069 Illegal instruction: 4 "$RUNTIME_PATH/dotnet" exec --runtimeconfig System.Runtime.Tests.runtimeconfig.json --depsfile System.Runtime.Tests.deps.json xunit.console.dll System.Runtime.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=AdditionalTimezoneChecks -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing $RSP_FILE
/private/tmp/helix/working/B38E099C/w/ACDA0999/e
----- end Mon Mar 27 20:14:10 EDT 2023 ----- exit code 132 ----------------------------------------------------------
exit code 132 means SIGILL Illegal Instruction. Core dumped. Likely codegen issue.
Assert failure(PID 12116 [0x00002f54], Thread: 4788 [0x12b4]): Assertion failed 'op1->gtEffectiveVal() == base' in 'System.RuntimeType:GetMethodBase(System.RuntimeType,int):System.Reflection.MethodBase' during 'Assertion prop' (IL size 480; hash 0xbca2468f; Tier1)
File: D:\a\_work\1\s\src\coreclr\jit\gentree.cpp Line: 4474

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

Seems like it does, but locally I am hitting another odd error. Running on a Coffee Lake i7 8700 so no AVX-512, I end up here when running under WSL2 when built against ed3721b.

cc @tannergooding

## JIT/opt/Compares/compares/compares.sh * thread #1, name = 'corerun', stop reason = signal SIGILL: illegal instruction operand
frame #0: 0x00007ffff776560d libcoreclr.so`RtlRestoreContext at context2.S:140
137 // See https://github.com/apple/darwin-xnu/blob/main/osfmk/i386/fpu.c#L174
138
139 // Restore the ZMM_Hi256 state
-> 140 vinsertf64x4 zmm0, zmm0, ymmword ptr [rdi + (CONTEXT_Zmm0H + 0 * 32)], 1
141 vinsertf64x4 zmm1, zmm1, ymmword ptr [rdi + (CONTEXT_Zmm0H + 1 * 32)], 1
142 vinsertf64x4 zmm2, zmm2, ymmword ptr [rdi + (CONTEXT_Zmm0H + 2 * 32)], 1
143 vinsertf64x4 zmm3, zmm3, ymmword ptr [rdi + (CONTEXT_Zmm0H + 3 * 32)], 1

@tannergooding

Copy link
Copy Markdown
Member

This would be caused by #83784

We're supposed to skip this bit if CONTEXT.XStateFeaturesMask hasn't been marked with XSTATE_MASK_AVX512: https://github.com/dotnet/runtime/pull/83784/files#diff-7fb8fda5dbdf85daa35acb2847f3613b2ca50e1dafafae5475e5443589258eacR128-R129

That correspondingly should only be set if FPREG_HasAvx512Registers returns true: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR964-R979

Which itself should only be getting set if the Linux kernel itself reports XFEATURE_MASK_AVX512 in the native thread context: https://github.com/dotnet/runtime/pull/83784/files#diff-6bd99da7ca73739fa65674c40dcb32ecc9af9bcaaff94fdc3d0f4e13704c2331R511-R525

and correspondingly the CPUID query also reports all 5 ISAs as supported: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR336-R383

Will need to dig into this a bit to try and repro things...

@tannergooding

Copy link
Copy Markdown
Member

Found the issue: #84012

Comment threadsrc/coreclr/jit/compiler.h Outdated
Comment on lines +1102 to +1109
(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);
}

bool lvNormalizeOnStore() const
{
return varTypeIsSmall(TypeGet()) &&
// lvIsStructField is treated the same as the aliased local, see fgDoNormalizeOnStore.
!(lvIsParam || m_addrExposed || lvIsStructField);
!(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It took me a while to guess that the reason is that the local could have been marked normalize-on-load in tier-0 due to the more conservative address exposure there. Maybe add a comment to that effect?

Also, I guess if we wanted to we could scope this down to just OSR locals that were address exposed in tier 0, since we have that information available.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about scoping it down, so let me do that (will need a new bit on lclvar dsc).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo, runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

So far just this one failure has recurred:

 Discovering: System.Runtime.Intrinsics.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Runtime.Intrinsics.Tests (found 1023 test cases)
Starting: System.Runtime.Intrinsics.Tests (parallel test collections = on, max threads = 2)
Assert failure(PID 23193 [0x00005a99], Thread: 23208 [0x5aa8]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
File: /__w/1/s/src/coreclr/jit/emitxarch.cpp Line: 7810
Image: /datadisks/disk1/work/B9AF0A02/p/dotnet

Does not repro locally, suspect it requires AVX-512.

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

set DOTNET_TieredCompilation=1
set DOTNET_ReadyToRun=0
set DOTNET_TC_QuickJitForLoops=1
set DOTNET_TieredPGO=1
set DOTNET_JitRandomGuardedDevirtualization=1
set DOTNET_JitRandomEdgeCounts=1
set DOTNET_JitRandomlyCollect64BitCounts=1
17:06:48.864 Running test: baseservices/threading/regressions/2164/foreground-shutdown/foreground-shutdown.cmd
Return code: 1
Raw output file: C:\h\w\BAA209F1\w\ACE309A2\uploads\regressions\2164\foreground-shutdown\output.txt
Raw output:
BEGIN EXECUTION
"C:\h\w\BAA209F1\p\corerun.exe" -p "System.Reflection.Metadata.MetadataUpdater.IsSupported=false" foreground-shutdown.dll Xunit.Sdk.EqualException: Assert.Equal() Failure
Expected: 100
Actual: 101
at Xunit.Assert.Equal[T](T expected, T actual, IEqualityComparer`1 comparer) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 96
at Xunit.Assert.Equal[T](T expected, T actual) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 63
at __GeneratedMainWrapper.Main()
Expected: 100
Actual: 101
END EXECUTION - FAILED

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

Bruce pointed out a mismatched VM altjit would work -- I tried this via windows crossjit on linux -- and while I do indeed see zmm's in the disasm, I can't yet repro the assertion failure.

; Assembly listing for method System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this
; Emitting BLENDED_CODE for X64 CPU with AVX512 - Windows
; Tier-1 compilation
; OSR variant for entry point 0x11f
...
4889842420010000 mov qword ptr [rsp+120H], rax
62F17C4810B42470030000 vmovups zmm6, zmmword ptr[rsp+370H]
8BB4246C030000 mov esi, dword ptr [rsp+36CH]
...

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

There aren't any OSR methods in this test, so the failure seems unrelated.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Going to merge since we have potential OSR silent bad code; will try and repro the AVX512 issue some other way.

@AndyAyersMS
AndyAyersMS merged commit fd157a5 into dotnet:mainMar 28, 2023
@AndyAyersMSAndyAyersMS mentioned this pull request Mar 30, 2023
72 tasks
@ghostghost locked as resolved and limited conversation to collaborators Apr 28, 2023
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 SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[jitosr_stress_random] Failures in System.Text related tests

3 participants

@AndyAyersMS@tannergooding@jakobbotsch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

JIT: small OSR locals must be normalize on load - #84000

Merged
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959
Mar 28, 2023
Merged

JIT: small OSR locals must be normalize on load#84000
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959

Conversation

@AndyAyersMS

@AndyAyersMSAndyAyersMS commented Mar 27, 2023

Copy link
Copy Markdown
Member

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0.

Fixes#83959.

When I changed the importation strategy for OSR in dotnet#83910 it
exposed a latent issue -- small OSR locals must normalized on load.
Fixesdotnet#83959.
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 27, 2023
@ghost

Copy link
Copy Markdown

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

Issue Details

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load.

Fixes #83959.

Author:AndyAyersMS
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

@jakobbotsch PTAL
cc @dotnet/jit-contrib

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Expecting a modest number of diffs for OSR methods.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

Running into various issues:

Assert failure(PID 24585 [0x00006009], Thread: 24602 [0x601a]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
;; osx
./RunTests.sh: line 168: 50069 Illegal instruction: 4 "$RUNTIME_PATH/dotnet" exec --runtimeconfig System.Runtime.Tests.runtimeconfig.json --depsfile System.Runtime.Tests.deps.json xunit.console.dll System.Runtime.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=AdditionalTimezoneChecks -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing $RSP_FILE
/private/tmp/helix/working/B38E099C/w/ACDA0999/e
----- end Mon Mar 27 20:14:10 EDT 2023 ----- exit code 132 ----------------------------------------------------------
exit code 132 means SIGILL Illegal Instruction. Core dumped. Likely codegen issue.
Assert failure(PID 12116 [0x00002f54], Thread: 4788 [0x12b4]): Assertion failed 'op1->gtEffectiveVal() == base' in 'System.RuntimeType:GetMethodBase(System.RuntimeType,int):System.Reflection.MethodBase' during 'Assertion prop' (IL size 480; hash 0xbca2468f; Tier1)
File: D:\a\_work\1\s\src\coreclr\jit\gentree.cpp Line: 4474

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

Seems like it does, but locally I am hitting another odd error. Running on a Coffee Lake i7 8700 so no AVX-512, I end up here when running under WSL2 when built against ed3721b.

cc @tannergooding

## JIT/opt/Compares/compares/compares.sh * thread #1, name = 'corerun', stop reason = signal SIGILL: illegal instruction operand
frame #0: 0x00007ffff776560d libcoreclr.so`RtlRestoreContext at context2.S:140
137 // See https://github.com/apple/darwin-xnu/blob/main/osfmk/i386/fpu.c#L174
138
139 // Restore the ZMM_Hi256 state
-> 140 vinsertf64x4 zmm0, zmm0, ymmword ptr [rdi + (CONTEXT_Zmm0H + 0 * 32)], 1
141 vinsertf64x4 zmm1, zmm1, ymmword ptr [rdi + (CONTEXT_Zmm0H + 1 * 32)], 1
142 vinsertf64x4 zmm2, zmm2, ymmword ptr [rdi + (CONTEXT_Zmm0H + 2 * 32)], 1
143 vinsertf64x4 zmm3, zmm3, ymmword ptr [rdi + (CONTEXT_Zmm0H + 3 * 32)], 1

@tannergooding

Copy link
Copy Markdown
Member

This would be caused by #83784

We're supposed to skip this bit if CONTEXT.XStateFeaturesMask hasn't been marked with XSTATE_MASK_AVX512: https://github.com/dotnet/runtime/pull/83784/files#diff-7fb8fda5dbdf85daa35acb2847f3613b2ca50e1dafafae5475e5443589258eacR128-R129

That correspondingly should only be set if FPREG_HasAvx512Registers returns true: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR964-R979

Which itself should only be getting set if the Linux kernel itself reports XFEATURE_MASK_AVX512 in the native thread context: https://github.com/dotnet/runtime/pull/83784/files#diff-6bd99da7ca73739fa65674c40dcb32ecc9af9bcaaff94fdc3d0f4e13704c2331R511-R525

and correspondingly the CPUID query also reports all 5 ISAs as supported: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR336-R383

Will need to dig into this a bit to try and repro things...

@tannergooding

Copy link
Copy Markdown
Member

Found the issue: #84012

Comment threadsrc/coreclr/jit/compiler.h Outdated
Comment on lines +1102 to +1109
(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);
}

bool lvNormalizeOnStore() const
{
return varTypeIsSmall(TypeGet()) &&
// lvIsStructField is treated the same as the aliased local, see fgDoNormalizeOnStore.
!(lvIsParam || m_addrExposed || lvIsStructField);
!(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It took me a while to guess that the reason is that the local could have been marked normalize-on-load in tier-0 due to the more conservative address exposure there. Maybe add a comment to that effect?

Also, I guess if we wanted to we could scope this down to just OSR locals that were address exposed in tier 0, since we have that information available.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about scoping it down, so let me do that (will need a new bit on lclvar dsc).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo, runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

So far just this one failure has recurred:

 Discovering: System.Runtime.Intrinsics.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Runtime.Intrinsics.Tests (found 1023 test cases)
Starting: System.Runtime.Intrinsics.Tests (parallel test collections = on, max threads = 2)
Assert failure(PID 23193 [0x00005a99], Thread: 23208 [0x5aa8]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
File: /__w/1/s/src/coreclr/jit/emitxarch.cpp Line: 7810
Image: /datadisks/disk1/work/B9AF0A02/p/dotnet

Does not repro locally, suspect it requires AVX-512.

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

set DOTNET_TieredCompilation=1
set DOTNET_ReadyToRun=0
set DOTNET_TC_QuickJitForLoops=1
set DOTNET_TieredPGO=1
set DOTNET_JitRandomGuardedDevirtualization=1
set DOTNET_JitRandomEdgeCounts=1
set DOTNET_JitRandomlyCollect64BitCounts=1
17:06:48.864 Running test: baseservices/threading/regressions/2164/foreground-shutdown/foreground-shutdown.cmd
Return code: 1
Raw output file: C:\h\w\BAA209F1\w\ACE309A2\uploads\regressions\2164\foreground-shutdown\output.txt
Raw output:
BEGIN EXECUTION
"C:\h\w\BAA209F1\p\corerun.exe" -p "System.Reflection.Metadata.MetadataUpdater.IsSupported=false" foreground-shutdown.dll Xunit.Sdk.EqualException: Assert.Equal() Failure
Expected: 100
Actual: 101
at Xunit.Assert.Equal[T](T expected, T actual, IEqualityComparer`1 comparer) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 96
at Xunit.Assert.Equal[T](T expected, T actual) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 63
at __GeneratedMainWrapper.Main()
Expected: 100
Actual: 101
END EXECUTION - FAILED

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

Bruce pointed out a mismatched VM altjit would work -- I tried this via windows crossjit on linux -- and while I do indeed see zmm's in the disasm, I can't yet repro the assertion failure.

; Assembly listing for method System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this
; Emitting BLENDED_CODE for X64 CPU with AVX512 - Windows
; Tier-1 compilation
; OSR variant for entry point 0x11f
...
4889842420010000 mov qword ptr [rsp+120H], rax
62F17C4810B42470030000 vmovups zmm6, zmmword ptr[rsp+370H]
8BB4246C030000 mov esi, dword ptr [rsp+36CH]
...

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

There aren't any OSR methods in this test, so the failure seems unrelated.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Going to merge since we have potential OSR silent bad code; will try and repro the AVX512 issue some other way.

@AndyAyersMS
AndyAyersMS merged commit fd157a5 into dotnet:mainMar 28, 2023
@AndyAyersMSAndyAyersMS mentioned this pull request Mar 30, 2023
72 tasks
@ghostghost locked as resolved and limited conversation to collaborators Apr 28, 2023
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 SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[jitosr_stress_random] Failures in System.Text related tests

3 participants

@AndyAyersMS@tannergooding@jakobbotsch
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

JIT: small OSR locals must be normalize on load - #84000

Merged
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959
Mar 28, 2023
Merged

JIT: small OSR locals must be normalize on load#84000
AndyAyersMS merged 3 commits into
dotnet:mainfrom
AndyAyersMS:Fix83959

Conversation

@AndyAyersMS

@AndyAyersMSAndyAyersMS commented Mar 27, 2023

Copy link
Copy Markdown
Member

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load if they were exposed at Tier0.

Fixes#83959.

When I changed the importation strategy for OSR in dotnet#83910 it
exposed a latent issue -- small OSR locals must normalized on load.
Fixesdotnet#83959.
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 27, 2023
@ghost

Copy link
Copy Markdown

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

Issue Details

When I changed the importation strategy for OSR in #83910 it exposed a latent issue -- small OSR locals must normalized on load.

Fixes #83959.

Author:AndyAyersMS
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

@jakobbotsch PTAL
cc @dotnet/jit-contrib

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Expecting a modest number of diffs for OSR methods.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

Running into various issues:

Assert failure(PID 24585 [0x00006009], Thread: 24602 [0x601a]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
;; osx
./RunTests.sh: line 168: 50069 Illegal instruction: 4 "$RUNTIME_PATH/dotnet" exec --runtimeconfig System.Runtime.Tests.runtimeconfig.json --depsfile System.Runtime.Tests.deps.json xunit.console.dll System.Runtime.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=AdditionalTimezoneChecks -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing $RSP_FILE
/private/tmp/helix/working/B38E099C/w/ACDA0999/e
----- end Mon Mar 27 20:14:10 EDT 2023 ----- exit code 132 ----------------------------------------------------------
exit code 132 means SIGILL Illegal Instruction. Core dumped. Likely codegen issue.
Assert failure(PID 12116 [0x00002f54], Thread: 4788 [0x12b4]): Assertion failed 'op1->gtEffectiveVal() == base' in 'System.RuntimeType:GetMethodBase(System.RuntimeType,int):System.Reflection.MethodBase' during 'Assertion prop' (IL size 480; hash 0xbca2468f; Tier1)
File: D:\a\_work\1\s\src\coreclr\jit\gentree.cpp Line: 4474

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

May also fix #83960 -- checking that now.

Seems like it does, but locally I am hitting another odd error. Running on a Coffee Lake i7 8700 so no AVX-512, I end up here when running under WSL2 when built against ed3721b.

cc @tannergooding

## JIT/opt/Compares/compares/compares.sh * thread #1, name = 'corerun', stop reason = signal SIGILL: illegal instruction operand
frame #0: 0x00007ffff776560d libcoreclr.so`RtlRestoreContext at context2.S:140
137 // See https://github.com/apple/darwin-xnu/blob/main/osfmk/i386/fpu.c#L174
138
139 // Restore the ZMM_Hi256 state
-> 140 vinsertf64x4 zmm0, zmm0, ymmword ptr [rdi + (CONTEXT_Zmm0H + 0 * 32)], 1
141 vinsertf64x4 zmm1, zmm1, ymmword ptr [rdi + (CONTEXT_Zmm0H + 1 * 32)], 1
142 vinsertf64x4 zmm2, zmm2, ymmword ptr [rdi + (CONTEXT_Zmm0H + 2 * 32)], 1
143 vinsertf64x4 zmm3, zmm3, ymmword ptr [rdi + (CONTEXT_Zmm0H + 3 * 32)], 1

@tannergooding

Copy link
Copy Markdown
Member

This would be caused by #83784

We're supposed to skip this bit if CONTEXT.XStateFeaturesMask hasn't been marked with XSTATE_MASK_AVX512: https://github.com/dotnet/runtime/pull/83784/files#diff-7fb8fda5dbdf85daa35acb2847f3613b2ca50e1dafafae5475e5443589258eacR128-R129

That correspondingly should only be set if FPREG_HasAvx512Registers returns true: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR964-R979

Which itself should only be getting set if the Linux kernel itself reports XFEATURE_MASK_AVX512 in the native thread context: https://github.com/dotnet/runtime/pull/83784/files#diff-6bd99da7ca73739fa65674c40dcb32ecc9af9bcaaff94fdc3d0f4e13704c2331R511-R525

and correspondingly the CPUID query also reports all 5 ISAs as supported: https://github.com/dotnet/runtime/pull/83784/files#diff-eb14b2a3e90e2d0548c465157fcf8480b33fa3b83c1594c068486fe894d32bfeR336-R383

Will need to dig into this a bit to try and repro things...

@tannergooding

Copy link
Copy Markdown
Member

Found the issue: #84012

Comment threadsrc/coreclr/jit/compiler.h Outdated
Comment on lines +1102 to +1109
(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);
}

bool lvNormalizeOnStore() const
{
return varTypeIsSmall(TypeGet()) &&
// lvIsStructField is treated the same as the aliased local, see fgDoNormalizeOnStore.
!(lvIsParam || m_addrExposed || lvIsStructField);
!(lvIsParam || m_addrExposed || lvIsStructField || lvIsOSRLocal);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It took me a while to guess that the reason is that the local could have been marked normalize-on-load in tier-0 due to the more conservative address exposure there. Maybe add a comment to that effect?

Also, I guess if we wanted to we could scope this down to just OSR locals that were address exposed in tier 0, since we have that information available.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about scoping it down, so let me do that (will need a new bit on lclvar dsc).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr libraries-pgo, runtime-coreclr pgostress

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

So far just this one failure has recurred:

 Discovering: System.Runtime.Intrinsics.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Runtime.Intrinsics.Tests (found 1023 test cases)
Starting: System.Runtime.Intrinsics.Tests (parallel test collections = on, max threads = 2)
Assert failure(PID 23193 [0x00005a99], Thread: 23208 [0x5aa8]): Assertion failed '!CodeGen::instIsFP(ins) && (EA_SIZE(attr) <= EA_32BYTE) && (reg != REG_NA)' in 'System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this' during 'Generate code' (IL size 296; hash 0xe5239cf9; Tier1-OSR)
File: /__w/1/s/src/coreclr/jit/emitxarch.cpp Line: 7810
Image: /datadisks/disk1/work/B9AF0A02/p/dotnet

Does not repro locally, suspect it requires AVX-512.

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

set DOTNET_TieredCompilation=1
set DOTNET_ReadyToRun=0
set DOTNET_TC_QuickJitForLoops=1
set DOTNET_TieredPGO=1
set DOTNET_JitRandomGuardedDevirtualization=1
set DOTNET_JitRandomEdgeCounts=1
set DOTNET_JitRandomlyCollect64BitCounts=1
17:06:48.864 Running test: baseservices/threading/regressions/2164/foreground-shutdown/foreground-shutdown.cmd
Return code: 1
Raw output file: C:\h\w\BAA209F1\w\ACE309A2\uploads\regressions\2164\foreground-shutdown\output.txt
Raw output:
BEGIN EXECUTION
"C:\h\w\BAA209F1\p\corerun.exe" -p "System.Reflection.Metadata.MetadataUpdater.IsSupported=false" foreground-shutdown.dll Xunit.Sdk.EqualException: Assert.Equal() Failure
Expected: 100
Actual: 101
at Xunit.Assert.Equal[T](T expected, T actual, IEqualityComparer`1 comparer) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 96
at Xunit.Assert.Equal[T](T expected, T actual) in /_/src/xunit.assert/Asserts/EqualityAsserts.cs:line 63
at __GeneratedMainWrapper.Main()
Expected: 100
Actual: 101
END EXECUTION - FAILED

@AndyAyersMS

AndyAyersMS commented Mar 28, 2023

Copy link
Copy Markdown
MemberAuthor

@tannergooding how can I repro this w/o having AVX-512 capable hardware?

Bruce pointed out a mismatched VM altjit would work -- I tried this via windows crossjit on linux -- and while I do indeed see zmm's in the disasm, I can't yet repro the assertion failure.

; Assembly listing for method System.Runtime.Intrinsics.Tests.Vectors.Vector512Tests:Vector512SByteShuffleOneInputWithLocalIndicesTest():this
; Emitting BLENDED_CODE for X64 CPU with AVX512 - Windows
; Tier-1 compilation
; OSR variant for entry point 0x11f
...
4889842420010000 mov qword ptr [rsp+120H], rax
62F17C4810B42470030000 vmovups zmm6, zmmword ptr[rsp+370H]
8BB4246C030000 mov esi, dword ptr [rsp+36CH]
...

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Also seeing an arm64 failure. Not sure it is related but will take a look.

There aren't any OSR methods in this test, so the failure seems unrelated.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Going to merge since we have potential OSR silent bad code; will try and repro the AVX512 issue some other way.

@AndyAyersMS
AndyAyersMS merged commit fd157a5 into dotnet:mainMar 28, 2023
@AndyAyersMSAndyAyersMS mentioned this pull request Mar 30, 2023
72 tasks
@ghostghost locked as resolved and limited conversation to collaborators Apr 28, 2023
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 SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[jitosr_stress_random] Failures in System.Text related tests

3 participants

@AndyAyersMS@tannergooding@jakobbotsch