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.

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 wasm32 as a target architecture for the SIMD feature #324

Merged
merged 2 commits into from
May 23, 2021

Conversation

roee88
Copy link
Contributor

@roee88 roee88 commented May 18, 2021

Which issue does this PR close?

Closes #316

Rationale for this change

packed_simd compiles to wasm32-wasi and wasm32-unknown-unknown targets (and includes CI against it). It will only work on WASM runtimes that support the WASM SIMD proposal but that's no reason to block it from arrow-rs side.

What changes are included in this PR?

Same as #303 but without limiting to specific architectures. The rationale is described in #316:

Optional features should not be limited to specific architectures in arrow-rs itself. The arrow-rs CI shouldn't run tests with combinations that are not known to work, but as a user I would expect that if I enable a feature then it's enabled (and might create a failure on its own if it doesn't work yet).

If this turns out to be a problem with the CI then I will revert to just adding wasm32 to build.rs.

Are there any user-facing changes?

No

Signed-off-by: roee88 <roee88@gmail.com>
Signed-off-by: roee88 <roee88@gmail.com>
@roee88 roee88 changed the title Enable wasm32 simd Enable wasm32 as a target architecture for the SIMD feature May 18, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #324 (c5788c5) into master (c863a2c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #324   +/-   ##
=======================================
  Coverage   82.52%   82.52%           
=======================================
  Files         162      162           
  Lines       44007    44007           
=======================================
  Hits        36316    36316           
  Misses       7691     7691           
Impacted Files Coverage Δ
arrow/src/buffer/ops.rs 96.96% <ø> (ø)
arrow/src/compute/kernels/aggregate.rs 73.40% <ø> (ø)
arrow/src/compute/kernels/arithmetic.rs 87.36% <ø> (ø)
arrow/src/compute/kernels/comparison.rs 95.84% <ø> (ø)
arrow/src/util/bit_util.rs 100.00% <ø> (ø)

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 c863a2c...c5788c5. Read the comment docs.

@jorgecarleitao
Copy link
Member

cc @houqp

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.

This seems like a fine change to me. As @jorgecarleitao probably found, this configuration was added in 73b7576 by @houqp so I think it would be good if he wanted to weigh in

@roee88
Copy link
Contributor Author

roee88 commented May 23, 2021

Sure. Though I don't think that the link you shared actually affects it. Originally there was no feature flag for simd and it was always enabled for x86 and x86_64. Then, a feature flag was added on top to allow disabling simd, it was then set to disabled by default, then cfg_aliases was used (no functionality change), and finally aarch64 was added to the list of architectures.

So I might be wrong but it all seems to be for historic reasons. However, this applies only as long as it's disabled by default. If at some point it will change to enable by default it would need to be enabled by default only for known supported architectures and disabled by default otherwise. Is that something that's possible to implement easily?

@alamb
Copy link
Contributor

alamb commented May 23, 2021

Yeah, this looks good to me

I tested (on a mac, with the most recently updated nightly buid)

cargo +nightly build  --target=wasm32-unknown-unknown  --features=simd   -p arrow

and it built 👍

@alamb alamb merged commit 91ef8e9 into apache:master May 23, 2021
@roee88 roee88 deleted the enable-wasm32-simd branch May 23, 2021 11:15
@houqp
Copy link
Member

houqp commented May 23, 2021

Apologize for missing the ping, yes, the cfg_aliases was added with the goal to not introduce any functioning change at the time. I am all for simplifying it to use the native feature flag and let build fail for unsupported architectures :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add wasm32 to the list of target architectures of the simd feature
5 participants