Skip to content

ARROW-11194: [Rust] Enable packed_simd for aarch64#9148

Closed
nevi-me wants to merge 2 commits intoapache:masterfrom
nevi-me:ARROW-11194
Closed

ARROW-11194: [Rust] Enable packed_simd for aarch64#9148
nevi-me wants to merge 2 commits intoapache:masterfrom
nevi-me:ARROW-11194

Conversation

@nevi-me
Copy link
Contributor

@nevi-me nevi-me commented Jan 9, 2021

packed_simd has support for aarch64 for the functions that we are using.

We can't test this feature yet, as we don't have aarch64 targets on our Rust CI yet. I however tested this on an ARM Mac device.
I perceive the build risk to be low, as I'm only enabling aarch64 in addition to the already used x86_64 target in packed_simd.

packed_simd has support for aarch64 for the functions that we are using.
@github-actions
Copy link

github-actions bot commented Jan 9, 2021

@nevi-me
Copy link
Contributor Author

nevi-me commented Jan 9, 2021

@alamb @jorgecarleitao I see decent performance increases in benchmarks, but because my device gets hot running the benchmarks, I'm opting to view them as not too reliable.

@nevi-me nevi-me requested review from alamb and jorgecarleitao and removed request for alamb January 9, 2021 20:01
@codecov-io
Copy link

codecov-io commented Jan 9, 2021

Codecov Report

Merging #9148 (181a78d) into master (08cccd6) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9148      +/-   ##
==========================================
- Coverage   81.81%   81.80%   -0.02%     
==========================================
  Files         214      214              
  Lines       51373    51383      +10     
==========================================
+ Hits        42033    42034       +1     
- Misses       9340     9349       +9     
Impacted Files Coverage Δ
rust/arrow/src/buffer.rs 97.72% <ø> (ø)
rust/arrow/src/compute/kernels/aggregate.rs 75.00% <ø> (ø)
rust/arrow/src/compute/kernels/arithmetic.rs 89.83% <ø> (ø)
rust/arrow/src/compute/kernels/comparison.rs 95.91% <ø> (ø)
rust/arrow/src/datatypes.rs 78.75% <ø> (ø)
rust/arrow/src/util/bit_util.rs 100.00% <ø> (ø)
rust/datafusion/src/datasource/csv.rs 65.00% <0.00%> (-16.25%) ⬇️
rust/datafusion/src/datasource/parquet.rs 94.77% <0.00%> (-1.44%) ⬇️
rust/parquet/src/encodings/encoding.rs 95.43% <0.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08cccd6...181a78d. Read the comment docs.

Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

LGTM.

@mqy
Copy link
Contributor

mqy commented Jan 10, 2021

#8891 added simd_x86.
How about add simd: { any(simd_x86, simd_aarch64) } in this PR?

@nevi-me
Copy link
Contributor Author

nevi-me commented Jan 10, 2021

How about merging simd_x86 and simd_aarch64 together, on the presumption that as long as we use the same library for both (packed_simd_2 now, stdsimd later), we don't need to split the architectures?

    cfg_aliases! {
        simd: { all(any(target_arch = "x86", target_arch = "x86_64", target_arch = "aarch64"), feature = "simd") },
    }

@mqy
Copy link
Contributor

mqy commented Jan 10, 2021

How about merging simd_x86 and simd_aarch64 together, on the presumption that as long as we use the same library for both (packed_simd_2 now, stdsimd later), we don't need to split the architectures?

    cfg_aliases! {
        simd: { all(any(target_arch = "x86", target_arch = "x86_64", target_arch = "aarch64"), feature = "simd") },
    }

+1 because the codes make no difference for all targets

@mqy
Copy link
Contributor

mqy commented Jan 10, 2021

List some possible TODO/discussion about the SIMD feature:

[1] Quote from https://en.wikipedia.org/wiki/SIMD:
Small-scale (64 or 128 bits) SIMD became popular on general-purpose CPUs in the early 1990s and continued through 1997 and later with Motion Video Instructions (MVI) for Alpha. SIMD instructions can be found, to one degree or another, on most CPUs

@nevi-me
Copy link
Contributor Author

nevi-me commented Jan 10, 2021

  • Is it make sense to enable the SIMD feature by default

No, because we want stable support by default, so simd is opt-in (ARROW-6717)

  • Change static configure to dynamic CPU feature detection

ARROW-6410 mainly deals with this.

I'd opt not to document anything on this PR, as we have activity on JIRA documenting our use of SIMD.
In the long run, we'd likely want SIMD to become stable, so that we can more freely implement SIMD versions of our code, without the nightly constraint.

@mqy
Copy link
Contributor

mqy commented Jan 10, 2021

@nevi-me got it, thanks!

Copy link
Contributor

@paddyhoran paddyhoran left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Seems like a good change to me

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.80%. Comparing base (08cccd6) to head (181a78d).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9148      +/-   ##
==========================================
- Coverage   81.81%   81.80%   -0.02%     
==========================================
  Files         214      214              
  Lines       51373    51383      +10     
==========================================
+ Hits        42033    42034       +1     
- Misses       9340     9349       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants