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

Minor: Document SIMD rationale and tips #6554

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+30 −0
Open

Minor: Document SIMD rationale and tips #6554

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions arrow/CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,36 @@ specific JIRA issues and reference them in these code comments. For example:
// This is not sound because .... see https://issues.apache.org/jira/browse/ARROW-nnnnn
```

### Usage if SIMD / Auto vectorization

This create does not use SIMD intrinsics (e.g. [`std::simd`] directly, but
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This create does not use SIMD intrinsics (e.g. [`std::simd`] directly, but
This crate does not use SIMD intrinsics (e.g. [`std::simd`] directly, but

instead relies on LLVM's auto-vectorization.
Copy link
Member

Choose a reason for hiding this comment

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

"... on the compiler's ..." ?

(in fact, vectorization could be applied on Rust MIR level, before LLVM?)

Copy link
Contributor

@tustvold tustvold Oct 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ill confess it is a while since i dug into rustc, but I would have thought MIR to be to high level to effectively perform auto-vectorisation which is extremely ISA specific, the best it could do would be to use LLVMs vector types, but general heiristics for doing this would be hard


SIMD intrinsics are difficult to maintain and can be difficult to reason about.
The auto-vectorizer in LLVM is quite good and often produces better code than
hand-written manual uses of SIMD. In fact, this crate used to to have a fair
Copy link
Member

Choose a reason for hiding this comment

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

stuterred "to"

amount of manual SIMD, and over time we've removed it as the auto-vectorized
code was faster.
Copy link
Member

Choose a reason for hiding this comment

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

was -> turned out ?


[`std::simd`]: https://doc.rust-lang.org/std/simd/index.html

LLVM is relatively good at vectorizing vertical operations provided:

1. No conditionals within the loop body
2. Not too much inlining , as the vectorizer gives up if the code is too complex
Copy link
Member

Choose a reason for hiding this comment

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

extra whitespace before ,

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
2. Not too much inlining , as the vectorizer gives up if the code is too complex
2. Not too much inlining necessary, as the vectorizer gives up if the code is too complex

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this changes the meaning, which is that over zealous use of inline can break the vectorizer

3. No bitwise horizontal reductions or masking
Copy link
Member

Choose a reason for hiding this comment

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

is "bitwise horizontal reductions" an obvious term?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a class of SIMD operations, I think if people don't know to what this refers, they probably aren't the audience for this

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @tustvold , i see your point.
OTOH, SIMD is widely known term and people may come to read this doc out of sheer interest how we think about simdizing the code. The term stands out from the rest of the text as less understood and https://www.google.com/search?q=bitwise+horizontal+reductions doesn't bring an obvious definition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could link to https://rust-lang.github.io/packed_simd/perf-guide/vert-hor-ops.html

4. You've enabled SIMD instructions in the target ISA (e.g. `target-cpu` `RUSTFLAGS` flag)
Copy link
Member

Choose a reason for hiding this comment

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

Prefer passive voice. "SIMD instructions are enabled in the target ISA"


The last point is especially important as the default `target-cpu` doesn't
support many SIMD instructions. See the Performance Tips section at the
end of <https://crates.io/crates/arrow>

To ensure your code is fully vectorized, we recommend getting familiar with
Copy link
Member

Choose a reason for hiding this comment

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

your code -> the code

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To ensure your code is fully vectorized, we recommend getting familiar with
To ensure your code is fully vectorized, we recommend becoming familiar with

tools like <https://rust.godbolt.org/> (again being sure to set `RUSTFLAGS`) and
Copy link
Member

Choose a reason for hiding this comment

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

again being sure to set RUSTFLAGS

requires to set RUSTFLAGS properly

only once you've exhausted that avenue think of reaching for manual SIMD.
Generally the hard part is getting the algorithm structured in such a way that
it can be vectorized, regardless of what goes and generates those instructions.
Copy link
Member

Choose a reason for hiding this comment

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

maybe

Suggested change
it can be vectorized, regardless of what goes and generates those instructions.
it can be vectorized, regardless of what generates those instructions.


# Releases and publishing to crates.io

Please see the [release](../dev/release/README.md) for details on how to create arrow releases
Loading