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.

Sign up for GitHub

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

Jump to bottom

ARROW-17598: [C++] Skip memory_benchmark if SIMD level is NEON #14036

Merged
merged 2 commits into from
Sep 14, 2022
Merged

ARROW-17598: [C++] Skip memory_benchmark if SIMD level is NEON #14036

merged 2 commits into from
Sep 14, 2022

Conversation

drin
Copy link
Contributor

@drin drin commented Sep 2, 2022

Added an extra check if SIMD_LEVEL is "NEON" because
arrow/io/memory_benchmark does not provide definitions for those SIMD
instructions.

@drin
[ARROW-17598]: extra guard for memory_benchmark
886929e 
Added an extra check if SIMD_LEVEL is "NEON" because
arrow/io/memory_benchmark does not provide definitions for those SIMD
instructions.
@github-actions
Copy link

github-actions bot commented Sep 2, 2022

https://issues.apache.org/jira/browse/ARROW-17598

@drin
Copy link
Contributor Author

drin commented Sep 2, 2022

as mentioned in ARROW-17598, this bug requires the flag: -DARROW_BUILD_BENCHMARKS_REFERENCE="ON" which I don't expect the CI to exercise. If building with that flag and with SIMD_LEVEL=NEON, using release mode as an example, the following file should no longer be generated: ./release/arrow-io-memory-benchmark

@cyb70289 cyb70289 self-requested a review September 5, 2022 03:07
@cyb70289
Copy link
Contributor

cyb70289 commented Sep 5, 2022

We do support Arm in memory benchmark. But the code looks obsolete and doesn't compile per my test with clang-12 aarch64.
https://github.com/apache/arrow/blob/master/cpp/src/arrow/io/memory_benchmark.cc#L157

It's okay to disable this test on Arm as a workaround.

@cyb70289
Copy link
Contributor

cyb70289 commented Sep 5, 2022 •
edited
Loading

Is the Lint CI error related to this PR?

EDIT: Looks like cmake format error.
https://github.com/apache/arrow/runs/8162000539?check_suite_focus=true#step:5:971

archery lint --cmake-format --fix will check and fix cmake format error automatically.

@drin
Copy link
Contributor Author

drin commented Sep 13, 2022

finally got the cmake-format linter to work

cyb70289
cyb70289 approved these changes Sep 14, 2022
View reviewed changes
Copy link
Contributor

@cyb70289 cyb70289 left a comment

Choose a reason for hiding this comment

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

+1
Thanks @drin !

@cyb70289
Copy link
Contributor

CI errors are not related

@cyb70289 cyb70289 merged commit 3338d99 into apache:master Sep 14, 2022
@ursabot
Copy link

ursabot commented Sep 14, 2022

Benchmark runs are scheduled for baseline = f4e79b1 and contender = 3338d99. 3338d99 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.07% ⬆️0.1%] test-mac-arm
[Failed ⬇️0.56% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.25% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 3338d998 ec2-t3-xlarge-us-east-2
[Failed] 3338d998 test-mac-arm
[Failed] 3338d998 ursa-i9-9960x
[Finished] 3338d998 ursa-thinkcentre-m75q
[Finished] f4e79b10 ec2-t3-xlarge-us-east-2
[Finished] f4e79b10 test-mac-arm
[Failed] f4e79b10 ursa-i9-9960x
[Finished] f4e79b10 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@drin drin deleted the ARROW-17598-memory-benchmark-without-neon branch September 14, 2022 19:06
zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
@drin @zagto
ARROW-17598: [C++] Skip memory_benchmark if SIMD level is NEON (apach…
f6c3eed 
…e#14036)

Added an extra check if SIMD_LEVEL is "NEON" because
arrow/io/memory_benchmark does not provide definitions for those SIMD
instructions.

Authored-by: Aldrin M <[email protected]>
Signed-off-by: Yibo Cai <[email protected]>
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
@drin @fatemehp
ARROW-17598: [C++] Skip memory_benchmark if SIMD level is NEON (apach…
cfa836e 
…e#14036)

Added an extra check if SIMD_LEVEL is "NEON" because
arrow/io/memory_benchmark does not provide definitions for those SIMD
instructions.

Authored-by: Aldrin M <[email protected]>
Signed-off-by: Yibo Cai <[email protected]>
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

Assignees
No one assigned
Labels
Component: C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@drin @cyb70289 @ursabot