GH-49325: [C++] Check if YMM register saving is OS enabled#49326
GH-49325: [C++] Check if YMM register saving is OS enabled#49326pitrou merged 1 commit intoapache:mainfrom
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
|
|
282cb6c to
afba88b
Compare
|
@github-actions crossbow submit -g cpp |
|
Revision: 87ff21c Submitted crossbow builds: ursacomputing/crossbow @ actions-895c581763 |
|
@github-actions crossbow submit win |
pitrou
left a comment
There was a problem hiding this comment.
LGTM and it seems to conform to Intel's own recommendations. Let's just wait for CI
|
Revision: 87ff21c Submitted crossbow builds: ursacomputing/crossbow @ actions-8b588b027e |
|
Actually it seems we should also check a flag for SSE. |
I don't think that's necessary, we consider SSE mandatory. |
raulcd
left a comment
There was a problem hiding this comment.
Not sure if that's relevant as I am not an expert on this area but should we also check Xmm for SSE?
I was reading the info shared on the issue and found they are checking it there:
https://github.com/google/cpu_features/blob/545431d64a43f683d75e51c36df19f90afe82752/src/impl_x86__base_implementation.inl#L403-L426
|
@pitrou I believe this is all OK |
|
ok, just saw the rest of the comments, seems fine as you are the experts here :) |
|
After merging your PR, Conbench analyzed the 1 benchmarking run that has been run so far on merge-commit aea1ad3. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 57 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
Current behaviour is not correct.
What changes are included in this PR?
Check if YMM register saving is enabled by the OS before enabling AVX detection.
Are these changes tested?
Hard to, because we cannot set the value being read manually.
Are there any user-facing changes?
No.