Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests exposing issue #60035 #60047

Closed
wants to merge 2 commits into from
Closed

Conversation

echesakov
Copy link
Contributor

@davidwrighton This is a prototype of the test as we discussed today.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@echesakov echesakov marked this pull request as draft October 6, 2021 02:24
@echesakov
Copy link
Contributor Author

/azp run runtime-coreclr jitstress-isas-x86

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@echesakov
Copy link
Contributor Author

echesakov commented Oct 6, 2021

@davidwrighton Do you think we need an image per ISA to be able to test disabling individual ISAs? For example, Helper.Sse2.dll would only call Sse2.IsSupported and not Avx.IsSupported

@echesakov
Copy link
Contributor Author

/azp run runtime-coreclr jitstress-isas-x86

@echesakov
Copy link
Contributor Author

/azp run runtime-coreclr jitstress-isas-arm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tannergooding
Copy link
Member

tannergooding commented Oct 8, 2021

I think it would be beneficial if we also had a simple test assembly that validates scenarios such as:

if (Environment.GetEnvironmentVariable("COMPlus_EnableSSE") == "0")
{
    assert(Sse.IsSupported == false);
    // same with downstream IsSupported checks for SSE2 through FMA
}

This should validate that the environment variables are working as expected and, at least as I understand it, should work correctly for anything that is an "opportunistic" ISA (that is, it wouldn't work for SSE/SSE2 under crossgen since they are "required", but would work for SSE3+).

@echesakov
Copy link
Contributor Author

@tannergooding Do you want such test assembly to be crossgen-d before running?

@tannergooding
Copy link
Member

@tannergooding Do you want such test assembly to be crossgen-d before running?

I'd think we'd want a version that validates things like COMPlus_EnableSSE and COMPlus_EnableSSE2 when crossgen is disabled (to validate they work as intended for customer testing scenarios).

However, I think we should generally be able to test that these switches (for non-baseline ISAs) work as intended under crossgen as well. At least as I remember everything working, there shouldn't be anything blocking this from working (but I may be forgetting some nuance of the crossgen setup here).

@echesakov
Copy link
Contributor Author

I'd think we'd want a version that validates things like COMPlus_EnableSSE and COMPlus_EnableSSE2 when crossgen is disabled (to validate they work as intended for customer testing scenarios).

Make sense, I will try to incorporate these tests here.

However, I think we should generally be able to test that these switches (for non-baseline ISAs) work as intended under crossgen as well. At least as I remember everything working, there shouldn't be anything blocking this from working (but I may be forgetting some nuance of the crossgen setup here).

Basically, you want in addition to COMPlus_EnableHWIntrinsic=0 also test disabling individual ISAs with COMPlus_Enable{ISA}=0 under crossgen, right? I think it should be also quite easy to extend the tests.

@tannergooding
Copy link
Member

Basically, you want in addition to COMPlus_EnableHWIntrinsic=0 also test disabling individual ISAs with COMPlus_Enable{ISA}=0 under crossgen, right

Right. These COMPlus_Enable* switches are effectively "public surface area" and we tell customers they can use them for testing scenarios. So I think it makes sense that we should test and validate they are working (and continue working) as we intended them.

@echesakov
Copy link
Contributor Author

Hmm, it doesn't look that these are working as expected.

If I disable SSE2 I have discrepancy between what a ReadyToRun image assumes and what *.IsSupported returns.

D:\echesako\src\runtime>set COMPlus
COMPlus_EnableSSE2=0

D:\echesako\src\runtime>%CORE_ROOT%\corerun.exe artifacts\tests\coreclr\windows.x64.checked\readytorun\Runtime_60035\Runtime_60035\Runtime_60035.dll
System.Runtime.Intrinsics.X86.Sse2.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Sse2.X64.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Sse3.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Ssse3.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Sse41.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Sse41.X64.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Sse42.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Sse42.X64.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Popcnt.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Popcnt.X64.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'

Similarly, for SSE4.1

D:\echesako\src\runtime>set COMPlus
COMPlus_EnableSSE41=0

D:\echesako\src\runtime>%CORE_ROOT%\corerun.exe artifacts\tests\coreclr\windows.x64.checked\readytorun\Runtime_60035\Runtime_60035\Runtime_60035.dll
System.Runtime.Intrinsics.X86.Sse41.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Sse41.X64.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Sse42.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Sse42.X64.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Popcnt.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'
System.Runtime.Intrinsics.X86.Popcnt.X64.IsSupported returns 'False' while a loaded into the process ReadyToRun image assumes the value being 'True'

It does work for optional ISAs though. Say, for AVX

D:\echesako\src\runtime>set COMPlus_EnableAVX=0

D:\echesako\src\runtime>%CORE_ROOT%\corerun.exe artifacts\tests\coreclr\windows.x64.checked\readytorun\Runtime_60035\Runtime_60035\Runtime_60035.dll

D:\echesako\src\runtime>echo %errorlevel%
100

D:\echesako\src\runtime>set COMPlus_EnableAVX=1

D:\echesako\src\runtime>%CORE_ROOT%\corerun.exe artifacts\tests\coreclr\windows.x64.checked\readytorun\Runtime_60035\Runtime_60035\Runtime_60035.dll

D:\echesako\src\runtime>echo %errorlevel%
100

cc @jkotas

@jkotas
Copy link
Member

jkotas commented Oct 8, 2021

Hmm, it doesn't look that these are working as expected.

The behavior that you are seeing is what I would expect given how it is implemented currently. R2R code does not pay attention to ISA env variables, and so it always returns the actual configuration of the machine.

I think we need to move the reading of the ISA env variables to VM to fix this discrepancy as I have said in #60035 (comment)

optional ISAs though. Say, for AVX

Nit: In crossgen terminology, these are called unsupportedInstructionSet. If a method depends on unsupportedInstructionSet ISA, crossgen does not compile it and it always fallbacks to JIT at runtime.

@echesakov
Copy link
Contributor Author

Nit: In crossgen terminology, these are called unsupportedInstructionSet. If a method depends on unsupportedInstructionSet ISA, crossgen does not compile it and it always fallbacks to JIT at runtime.

Got it, thanks for explanation!

@echesakov
Copy link
Contributor Author

echesakov commented Oct 8, 2021

One more question for my education while we are discussing the current behavior.

Should I expect that under TieredCompilation=0 these tests will pass?

D:\echesako\src\runtime>set COMPlus
COMPlus_EnableSSE41=0
COMPlus_TieredCompilation=0

D:\echesako\src\runtime>%CORE_ROOT%\corerun.exe artifacts\tests\coreclr\windows.x64.checked\readytorun\Runtime_60035\Runtime_60035\Runtime_60035.dll

D:\echesako\src\runtime>echo %errorlevel%
100

@jkotas
Copy link
Member

jkotas commented Oct 8, 2021

No. There should be no observable behavior changes in hardware intrinsic APIs from turning tiered compilation on/off. If you see observable behavior changes in hardware intrinsic APIs when turning tiered compilation on/off, it is a bug.

@echesakov
Copy link
Contributor Author

No. There should be no observable behavior changes in hardware intrinsic APIs from turning tiered compilation on/off. If you see observable behavior changes in hardware intrinsic APIs when turning tiered compilation on/off, it is a bug.

It looks that in the TC=0 case we inline a pre-jitted method body, hence, it correctly returns right value of {ISA}.IsSupported while in the TC=1 case we keep calling it directly.

@echesakov
Copy link
Contributor Author

/azp run runtime-coreclr jitstress-isas-x86

@echesakov
Copy link
Contributor Author

/azp run runtime-coreclr jitstress-isas-arm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@echesakov
Copy link
Contributor Author

@tannergooding I updated the test generator and the tests - is this what you had in mind in #60047 (comment) ?

@@ -0,0 +1,319 @@
# Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid adding more Python to the repo? Can this be written in C# instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I could've written the generator in C# - I just picked Python since it was faster for me to iterate on the changes.

@ghost
Copy link

ghost commented Nov 14, 2021

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

@echesakov
Copy link
Contributor Author

echesakov commented Nov 15, 2021

I will work on re-writing in C# (as Jan requsted) and minimizing (as David requested) the tests later when I have bandwidth.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants