Skip to content

GH-40806: [C++] Correctly report asimd/neon in GetRuntimeInfo#40857

Merged
cyb70289 merged 2 commits intoapache:mainfrom
amoeba:cpu-info-asimd
Mar 28, 2024
Merged

GH-40806: [C++] Correctly report asimd/neon in GetRuntimeInfo#40857
cyb70289 merged 2 commits intoapache:mainfrom
amoeba:cpu-info-asimd

Conversation

@amoeba
Copy link
Copy Markdown
Member

@amoeba amoeba commented Mar 28, 2024 •
edited by github-actions bot
Loading

What changes are included in this PR?

New case to conditional in MakeSimdLevelString which makes GetRuntimeInfo report correctly on respective CPUs. I chose to have it report "neon". Lowercase to match other strings and "neon" instead of "asimd" because I think that makes more sense to users. I'm not 100% sure which is more correct.

Fixes #40806

Are these changes tested?

We don't have automated tests for this. I did install the R package and, on my M1 laptop it reports 'neon' now instead of 'none' before:

> arrow_info()
...
SIMD Level          neon
Detected SIMD Level neon

Are there any user-facing changes?

No.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 28, 2024

⚠️ GitHub issue #40806 has been automatically assigned in GitHub to PR creator.

@amoeba amoeba requested a review from cyb70289 March 28, 2024 03:37
@github-actions github-actions bot added the awaiting review Awaiting review label Mar 28, 2024
@amoeba amoeba requested a review from mapleFU March 28, 2024 03:37
@mapleFU
Copy link
Copy Markdown
Member

mapleFU commented Mar 28, 2024

Looks ok to me, but I'm not so familiar with arm. Will wait for yibo's comments

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Mar 28, 2024
@cyb70289 cyb70289 merged commit 6cecbab into apache:main Mar 28, 2024
@cyb70289
Copy link
Copy Markdown
Contributor

cyb70289 commented Mar 28, 2024

Thanks @amoeba !

@amoeba
Copy link
Copy Markdown
Member Author

amoeba commented Mar 28, 2024

Thanks @cyb70289 and @mapleFU!

@conbench-apache-arrow
Copy link
Copy Markdown

conbench-apache-arrow bot commented Mar 29, 2024

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 6cecbab.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 27 possible false positives for unstable benchmarks that are known to sometimes produce them.

@pitrou
Copy link
Copy Markdown
Member

pitrou commented Apr 3, 2024

Hello,
There are two problems with this PR:

  1. the possible values should reflect the allowed values for ARROW_USER_SIMD_LEVEL (which "neon" is not part of)
  2. it broke the arm64 wheel tests:
=================================== FAILURES ===================================
______________________________ test_runtime_info _______________________________

    def test_runtime_info():
        info = pa.runtime_info()
        assert isinstance(info, pa.RuntimeInfo)
        possible_simd_levels = ('none', 'sse4_2', 'avx', 'avx2', 'avx512')
>       assert info.simd_level in possible_simd_levels
E       AssertionError: assert 'neon' in ('none', 'sse4_2', 'avx', 'avx2', 'avx512')
E        +  where 'neon' = RuntimeInfo(simd_level='neon', detected_simd_level='neon').simd_level

Can this perhaps be reverted?

@cyb70289
Copy link
Copy Markdown
Contributor

cyb70289 commented Apr 3, 2024

oops, then it should be reverted

pitrou added a commit that referenced this pull request Apr 3, 2024
@pitrou
Revert "GH-40806: [C++] Correctly report asimd/neon in GetRuntimeInfo (…
1dc5dbc 
…#40857)"

This reverts commit 6cecbab.
@pitrou
Copy link
Copy Markdown
Member

pitrou commented Apr 3, 2024

Ok, I've opened #40980 to revert.

@amoeba
Copy link
Copy Markdown
Member Author

amoeba commented Apr 3, 2024

Thanks for catching this @pitrou.

@pitrou pitrou mentioned this pull request Apr 4, 2024
Merged
pitrou added a commit that referenced this pull request Apr 4, 2024
@pitrou
GH-40806: [C++] Revert changes from PR #40857 (#40980)
26631d7 
Revert changes from #40857.

`GetRuntimeInfo` returns the SIMD level for dynamic dispatch, but Neon currently does not participate in dynamic dispatch (actually, Neon should be available by default on all modern Arm CPUs AFAIU).

Authored-by: Antoine Pitrou <pitrou@free.fr>
Signed-off-by: Antoine Pitrou <antoine@python.org>
vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
@amoeba @vibhatha
apacheGH-40806: [C++] Correctly report asimd/neon in GetRuntimeInfo (a…
f9ddfdc 
…pache#40857)

### What changes are included in this PR?

New case to conditional in `MakeSimdLevelString` which makes
`GetRuntimeInfo` report correctly on respective CPUs. I chose to have it
report "neon". Lowercase to match other strings and "neon" instead of
"asimd" because I think that makes more sense to users. I'm not 100%
sure which is more correct.

Fixes apache#40806

### Are these changes tested?

We don't have automated tests for this. I did install the R package and,
on my M1 laptop it reports 'neon' now instead of 'none' before:

```r
> arrow_info()
...
SIMD Level          neon
Detected SIMD Level neon
```

### Are there any user-facing changes?

No.
* GitHub Issue: apache#40806
vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
@pitrou @vibhatha
apacheGH-40806: [C++] Revert changes from PR apache#40857 (apache#40980)
502a93b 
Revert changes from apache#40857.

`GetRuntimeInfo` returns the SIMD level for dynamic dispatch, but Neon currently does not participate in dynamic dispatch (actually, Neon should be available by default on all modern Arm CPUs AFAIU).

Authored-by: Antoine Pitrou <pitrou@free.fr>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cyb70289 cyb70289 cyb70289 approved these changes

@mapleFU mapleFU Awaiting requested review from mapleFU

Assignees

No one assigned

Labels

awaiting committer review Awaiting committer review Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++] Add support to GetRuntimeInfo for reporting ASIMD/NEON

4 participants

@amoeba @mapleFU @cyb70289 @pitrou