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

Enable FEATURE_SIMD and FEATURE_AVX_SUPPORT in the JIT #981

Merged
merged 1 commit into from May 12, 2015

Conversation

Projects
None yet
7 participants
@CarolEidt
Member

CarolEidt commented May 12, 2015

For amd64 processors, the JIT supports SIMD intrinsics and supports generating AVX instructions when the processor support is available.

Fix # 977

@CarolEidt

This comment has been minimized.

Show comment
Hide comment
@CarolEidt

CarolEidt May 12, 2015

Member

@AndreyAkinshin @LLITCHEV @sivarv - this addresses both the recognition of SIMD intrinsics, as well as then generation of AVX instructions (issue #977)

Member

CarolEidt commented May 12, 2015

@AndreyAkinshin @LLITCHEV @sivarv - this addresses both the recognition of SIMD intrinsics, as well as then generation of AVX instructions (issue #977)

@LLITCHEV

This comment has been minimized.

Show comment
Hide comment
@LLITCHEV

LLITCHEV May 12, 2015

Contributor

Looks fine to me.
Could you please open an issue to enable this on System V systems as well? Thanks!

Contributor

LLITCHEV commented May 12, 2015

Looks fine to me.
Could you please open an issue to enable this on System V systems as well? Thanks!

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas May 12, 2015

Member

As far as I can tell, this change is enabling it for all platforms.

We should add the SIMD/AVX tests as part of enabling this feature. This is especially important for Unix because of this code will be running on Unix for the first time.

Member

jkotas commented May 12, 2015

As far as I can tell, this change is enabling it for all platforms.

We should add the SIMD/AVX tests as part of enabling this feature. This is especially important for Unix because of this code will be running on Unix for the first time.

@CarolEidt

This comment has been minimized.

Show comment
Hide comment
@CarolEidt

CarolEidt May 12, 2015

Member

I am working on adding SIMD testing. It requires System.Numerics.Vectors.dll, and so I need to determine the best mechanism for sync'ing the CoreCLR and corefx support. I plan to sync up next week with @ramarag and @mellinoe to figure out the best approach - would you prefer that I hold off on this PR until then?

Member

CarolEidt commented May 12, 2015

I am working on adding SIMD testing. It requires System.Numerics.Vectors.dll, and so I need to determine the best mechanism for sync'ing the CoreCLR and corefx support. I plan to sync up next week with @ramarag and @mellinoe to figure out the best approach - would you prefer that I hold off on this PR until then?

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas May 12, 2015

Member

Good to hear that you are working on adding SIMD testing. As long as the change does not introduce major break on Unix, I am fine with it.

Member

jkotas commented May 12, 2015

Good to hear that you are working on adding SIMD testing. As long as the change does not introduce major break on Unix, I am fine with it.

@LLITCHEV

This comment has been minimized.

Show comment
Hide comment
@LLITCHEV

LLITCHEV May 12, 2015

Contributor

I agree with Jan that we need to very thoroughly test the functionality before we enable it on System V systems and have all the tests running there. That is why I asked Carol to create an issue for enabling it on Unix (and that will include doing the necessary testing and creating the necessary tests.)
As it is, the SIMD and AVX features seems to be disabled on System V OSs. CMake's value for CMAKE_SYSTEM_PROCESSOR (on Linux) is "x86_64" (a 64 bit, x86 processor.)

Contributor

LLITCHEV commented May 12, 2015

I agree with Jan that we need to very thoroughly test the functionality before we enable it on System V systems and have all the tests running there. That is why I asked Carol to create an issue for enabling it on Unix (and that will include doing the necessary testing and creating the necessary tests.)
As it is, the SIMD and AVX features seems to be disabled on System V OSs. CMake's value for CMAKE_SYSTEM_PROCESSOR (on Linux) is "x86_64" (a 64 bit, x86 processor.)

@briansull

This comment has been minimized.

Show comment
Hide comment
@briansull

briansull May 12, 2015

Contributor

LGTM

Contributor

briansull commented May 12, 2015

LGTM

1 similar comment
@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin
Member

AndreyAkinshin commented May 12, 2015

LGTM

@LLITCHEV

This comment has been minimized.

Show comment
Hide comment
@LLITCHEV

LLITCHEV May 12, 2015

Contributor

After talking to Carol, I have a little suggestion.

Although it is clear that the features are disabled on Unix, it is confusing to why.
I’d make the code look something like the following, so it is more clear.
If (Win32)
if (IS_64BIT_BUILD EQUAL 1)
add_definitions(-DFEATURE_SIMD -DFEATURE_AVX_SUPPORT)
endif (IS_64BIT_BUILD EQUAL 1)
endif (Win32)

This hopefully removes any confusion.

Contributor

LLITCHEV commented May 12, 2015

After talking to Carol, I have a little suggestion.

Although it is clear that the features are disabled on Unix, it is confusing to why.
I’d make the code look something like the following, so it is more clear.
If (Win32)
if (IS_64BIT_BUILD EQUAL 1)
add_definitions(-DFEATURE_SIMD -DFEATURE_AVX_SUPPORT)
endif (IS_64BIT_BUILD EQUAL 1)
endif (Win32)

This hopefully removes any confusion.

Enable FEATURE_SIMD and FEATURE_AVX_SUPPORT in the JIT
For amd64 processors, the JIT supports SIMD intrinsics and supports generating AVX instructions when the processor support is available.

Fix # 977
@LLITCHEV

This comment has been minimized.

Show comment
Hide comment
@LLITCHEV

LLITCHEV May 12, 2015

Contributor

Looks fine to me.

Contributor

LLITCHEV commented May 12, 2015

Looks fine to me.

CarolEidt added a commit that referenced this pull request May 12, 2015

Merge pull request #981 from CarolEidt/EnableSIMDAndAVX
Enable FEATURE_SIMD and FEATURE_AVX_SUPPORT in the JIT

@CarolEidt CarolEidt merged commit 8e8a45d into dotnet:master May 12, 2015

1 check passed

default Build finished.
Details

@CarolEidt CarolEidt deleted the CarolEidt:EnableSIMDAndAVX branch May 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment