Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Enable Vector256<T> on AVX CPUs #15528

Merged
merged 1 commit into from
Dec 16, 2017
Merged

Enable Vector256<T> on AVX CPUs #15528

merged 1 commit into from
Dec 16, 2017

Conversation

fiigii
Copy link

@fiigii fiigii commented Dec 14, 2017

Enable Vector256<T> on AVX CPUs (Sandy Bridge and Ivy Bridge).

Fix #15519. OSX CI looks like running on an old machine sometimes.

@fiigii fiigii changed the title Enable Vector256<T> on AVX CPUs [WIP] Enable Vector256<T> on AVX CPUs Dec 14, 2017
@fiigii fiigii closed this Dec 15, 2017
@fiigii fiigii reopened this Dec 15, 2017
@fiigii fiigii changed the title [WIP] Enable Vector256<T> on AVX CPUs Enable Vector256<T> on AVX CPUs Dec 15, 2017
@fiigii
Copy link
Author

fiigii commented Dec 15, 2017

@jkotas @CarolEidt @BruceForstall PTAL

@fiigii
Copy link
Author

fiigii commented Dec 15, 2017

I have tested with COMPlus_EnableAVX2=0, it works fine now.

@@ -7729,7 +7729,19 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
// The minimum and maximum possible number of bytes in a SIMD vector.
unsigned int maxSIMDStructBytes()
{
#if FEATURE_HW_INTRINSICS && defined(_TARGET_XARCH_)

Choose a reason for hiding this comment

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

This could be confusing, so I would suggest adding a comment to the effect that in the case of AVX without AVX2, Vector<T> will be limited to 16 bytes, even though the full 32 byte AVX register length is supported for the HW intrinsics.

Copy link

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM, but I think a comment would help clarify.

@fiigii
Copy link
Author

fiigii commented Dec 16, 2017

@CarolEidt Thank you for the suggestion. I added comments for getSIMDVectorRegisterByteLength and maxSIMDStructBytes to clarify their usage.

Copy link

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

Excellent - thanks!

@CarolEidt CarolEidt merged commit a1bc40d into dotnet:master Dec 16, 2017
@fiigii fiigii deleted the simd32 branch December 16, 2017 07:36
@BruceForstall
Copy link
Member

@fiigii @CarolEidt Is there an issue to track how we test all the new SIMD modes?

E.g., we currently have the following SIMD-specific modes in Jenkins:

['COMPlus_EnableAVX' : '0', 'COMPlus_EnableSSE3_4' : '0'],
['COMPlus_FeatureSIMD' : '0'],

(from netci.groovy)

What more do we need?

Are the CI machines even capable of testing this stuff?

@fiigii
Copy link
Author

fiigii commented Dec 20, 2017

Is there an issue to track how we test all the new SIMD modes?

Not yet.
After #15514 get merged, HW intrinsic tests require COMPlus_EnableIncompleteISAClass=1.

@fiigii
Copy link
Author

fiigii commented Dec 20, 2017

I think we need the three CI jobs (at least) with Debug and Checked build.

['COMPlus_EnableIncompleteISAClass' : '1'], 
['COMPlus_EnableIncompleteISAClass' : '1', 'COMPlus_EnableAVX' : '0'], // non-Vex-encoding
['COMPlus_EnableIncompleteISAClass' : '1', 'COMPlus_EnableAVX2' : '0'], // for Sandy Bridge and Ivy Bridge
['COMPlus_EnableIncompleteISAClass' : '1', 'COMPlus_FeatureSIMD' : '0'], // No SIMD

@CarolEidt
Copy link

What more do we need?

I think that we will need to have the ability to turn off all the various COMPlus_EnableXXX flags. At least for the time being, they won't have any impact except on the tests that explicitly test for them, so they should be selectively applied, however that might be made to work.

Are the CI machines even capable of testing this stuff?

I believe that most of our CI machines now have AVX2. However, I don't think any of the tests require it, so if we happened to run the tests on, say, a non-AVX2 machine, the tests run with COMPlus_EnableAVX2=0 would simply be duplicative of those with it on.

@BruceForstall
Copy link
Member

Unrelated: @fiigii These tests are failing in the CI:

JIT_HardwareIntrinsics._IsSupported_ro_IsSupported_ro_._IsSupported_ro_IsSupported_ro_cmd
JIT_HardwareIntrinsics._IsSupported_r_IsSupported_r_._IsSupported_r_IsSupported_r_cmd

in the x64 Checked Windows job with COMPlus_FeatureSIMD=0. Is this due to one of your changes? If so, can you please fix ASAP?

See:
https://ci.dot.net/job/dotnet_coreclr/job/master/job/jitstress/job/x64_checked_windows_nt_jitnosimd/39

@fiigii
Copy link
Author

fiigii commented Dec 20, 2017

@BruceForstall the current "IsSupported" tests make no sense with those environment variables, so I deleted them in #15514.

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.

4 participants