You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "felipecrv (via GitHub)" <gi...@apache.org> on 2024/04/05 00:28:13 UTC
[PR] Compiler assume [arrow]
felipecrv opened a new pull request, #41021:
URL: https://github.com/apache/arrow/pull/41021
### Rationale for this change
Allow portable use of the "assumption" feature of modern GCC, Clang, and MSVC compilers.
### What changes are included in this PR?
- Documenting of macros in `macros.h`
- Addition of `ARROW_COMPILER_ASSUME`
- One simple change where this macro simplifies the existing code while keeping the common case of a loop branch dependent only on local variables
### Are these changes tested?
By existing tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #41021:
URL: https://github.com/apache/arrow/pull/41021#issuecomment-2039987936
> I wonder would llvm / gcc have a pass or other things finding out that could it find the assertions? Seems that the rule in this case is trivial, maybe we can use this when it's not so trivial and have some benchmark shows that performance would benifits from this?
Yes, they already do a great job finding assumptions [1] and the C++ standard allows many assumptions to be made. e.g. every pointer dereference assumes that the pointer is not-null, divisors are assumed to be non-0, signed arithmetic is assumed to not overflow, etc.
[1] assertions and assumptions are very different things -> https://blog.regehr.org/archives/1096
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1556051764
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,40 +36,63 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
Review Comment:
Yeah but let benchmark decides, I think it would be useful
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #41021:
URL: https://github.com/apache/arrow/pull/41021#issuecomment-2045884236
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit aeb1618a3093bc05cd81edbc0f1de54f8c7f406b.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/23628810624) has more details. It also includes information about 12 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1553637741
##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -181,14 +181,13 @@ class ARROW_EXPORT VarLengthListLikeBuilder : public ArrayBuilder {
if constexpr (is_list_view(TYPE::type_id)) {
sizes = array.GetValues<offset_type>(2);
}
+ const bool has_validity = array.buffers[0].data != NULLPTR;
const bool all_valid = !array.MayHaveLogicalNulls();
- const uint8_t* validity = array.HasValidityBitmap() ? array.buffers[0].data : NULLPTR;
ARROW_RETURN_NOT_OK(Reserve(length));
for (int64_t row = offset; row < offset + length; row++) {
- const bool is_valid =
- all_valid || (validity && bit_util::GetBit(validity, array.offset + row)) ||
- array.IsValid(row);
Review Comment:
`IsValid` gets inlined. The assumptions means when `IsValid` is expanded here, `if (this->buffers[0].data != NULLPTR) {` becomes `if (has_validity)`
```cpp
inline bool IsValid(int64_t i) const {
if (this->buffers[0].data != NULLPTR) {
return bit_util::GetBit(this->buffers[0].data, i + this->offset);
} else {
const auto type = this->type->id();
if (type == Type::SPARSE_UNION) {
return !IsNullSparseUnion(i);
}
if (type == Type::DENSE_UNION) {
return !IsNullDenseUnion(i);
}
if (type == Type::RUN_END_ENCODED) {
return !IsNullRunEndEncoded(i);
}
return this->null_count != this->length;
}
}
```
The place with the biggest impact (in another PR) is where I specialize the entire loop based on the array having a null bitmap from the beggining:
```cpp
ARROW_COMPILER_ASSUME(array.buffers[0].data != nullptr);
if (array.IsValid(i)) {
...
```
`IsValid` gets inlined and much smaller:
```diff
inline bool IsValid(int64_t i) const {
- if (this->buffers[0].data != NULLPTR) {
return bit_util::GetBit(this->buffers[0].data, i + this->offset);
- } else {
- const auto type = this->type->id();
- if (type == Type::SPARSE_UNION) {
- return !IsNullSparseUnion(i);
- }
- if (type == Type::DENSE_UNION) {
- return !IsNullDenseUnion(i);
- }
- if (type == Type::RUN_END_ENCODED) {
- return !IsNullRunEndEncoded(i);
- }
- return this->null_count != this->length;
- }
- }
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1553189631
##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -181,14 +181,13 @@ class ARROW_EXPORT VarLengthListLikeBuilder : public ArrayBuilder {
if constexpr (is_list_view(TYPE::type_id)) {
sizes = array.GetValues<offset_type>(2);
}
+ const bool has_validity = array.buffers[0].data != NULLPTR;
const bool all_valid = !array.MayHaveLogicalNulls();
- const uint8_t* validity = array.HasValidityBitmap() ? array.buffers[0].data : NULLPTR;
ARROW_RETURN_NOT_OK(Reserve(length));
for (int64_t row = offset; row < offset + length; row++) {
- const bool is_valid =
- all_valid || (validity && bit_util::GetBit(validity, array.offset + row)) ||
- array.IsValid(row);
int64_t size = 0;
+ ARROW_COMPILER_ASSUME(has_validity == (array.buffers[0].data != NULLPTR));
Review Comment:
Same question. It seems entirely redundant and therefore useless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1553737891
##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -181,14 +181,13 @@ class ARROW_EXPORT VarLengthListLikeBuilder : public ArrayBuilder {
if constexpr (is_list_view(TYPE::type_id)) {
sizes = array.GetValues<offset_type>(2);
}
+ const bool has_validity = array.buffers[0].data != NULLPTR;
const bool all_valid = !array.MayHaveLogicalNulls();
- const uint8_t* validity = array.HasValidityBitmap() ? array.buffers[0].data : NULLPTR;
ARROW_RETURN_NOT_OK(Reserve(length));
for (int64_t row = offset; row < offset + length; row++) {
- const bool is_valid =
- all_valid || (validity && bit_util::GetBit(validity, array.offset + row)) ||
- array.IsValid(row);
int64_t size = 0;
+ ARROW_COMPILER_ASSUME(has_validity == (array.buffers[0].data != NULLPTR));
Review Comment:
> Same question. It seems entirely redundant and therefore useless.
I agree it's not a strong example of the use of this macro, I have a better example that would create conflicts with the work I'm doing on another PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1553770296
##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -181,14 +181,13 @@ class ARROW_EXPORT VarLengthListLikeBuilder : public ArrayBuilder {
if constexpr (is_list_view(TYPE::type_id)) {
sizes = array.GetValues<offset_type>(2);
}
+ const bool has_validity = array.buffers[0].data != NULLPTR;
const bool all_valid = !array.MayHaveLogicalNulls();
- const uint8_t* validity = array.HasValidityBitmap() ? array.buffers[0].data : NULLPTR;
ARROW_RETURN_NOT_OK(Reserve(length));
for (int64_t row = offset; row < offset + length; row++) {
- const bool is_valid =
- all_valid || (validity && bit_util::GetBit(validity, array.offset + row)) ||
- array.IsValid(row);
int64_t size = 0;
+ ARROW_COMPILER_ASSUME(has_validity == (array.buffers[0].data != NULLPTR));
Review Comment:
I have no problem with the macros. It's just that the example use in this PR seems a bit artificial.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1556336474
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,41 +36,62 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
//
-#if defined(__GNUC__)
-#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
-#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
+// [1] https://lobste.rs/s/uwgtkt/don_t_use_likely_unlikely_attributes#c_xi3wmc
+// [2] "Portable assumptions"
+// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf
+// [3] "Assertions Are Pessimistic, Assumptions Are Optimistic"
+// https://blog.regehr.org/archives/1096
+#define ARROW_UNUSED(x) (void)(x)
+#define ARROW_ARG_UNUSED(x)
+#if defined(__GNUC__) // GCC and clang
#define ARROW_NORETURN __attribute__((noreturn))
#define ARROW_NOINLINE __attribute__((noinline))
#define ARROW_FORCE_INLINE __attribute__((always_inline))
+#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
+#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
#define ARROW_PREFETCH(addr) __builtin_prefetch(addr)
-#elif defined(_MSC_VER)
+#define ARROW_RESTRICT __restrict
+#if defined(__clang__) // clang-specific
+#define ARROW_COMPILER_ASSUME(expr) __builtin_assume(expr)
+#else
+#define ARROW_COMPILER_ASSUME(expr) \
+ if (!(expr)) { \
+ __builtin_unreachable(); \
Review Comment:
Ok, thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv merged PR #41021:
URL: https://github.com/apache/arrow/pull/41021
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1556368340
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,40 +36,63 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
//
+// [1] https://lobste.rs/s/uwgtkt/don_t_use_likely_unlikely_attributes#c_xi3wmc
+// [2] "Portable assumptions"
+// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf
+// [3] "Assertions Are Pessimistic, Assumptions Are Optimistic"
+// https://blog.regehr.org/archives/1096
+#define ARROW_UNUSED(x) (void)(x)
+#define ARROW_ARG_UNUSED(x)
#if defined(__GNUC__)
+// Both GCC and clang define __GNUC__
Review Comment:
I changed the comment :)
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,40 +36,63 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
Review Comment:
I greatly expanded the comments here. Pushing now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1556335757
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,41 +36,62 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
//
-#if defined(__GNUC__)
-#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
-#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
+// [1] https://lobste.rs/s/uwgtkt/don_t_use_likely_unlikely_attributes#c_xi3wmc
+// [2] "Portable assumptions"
+// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf
+// [3] "Assertions Are Pessimistic, Assumptions Are Optimistic"
+// https://blog.regehr.org/archives/1096
+#define ARROW_UNUSED(x) (void)(x)
+#define ARROW_ARG_UNUSED(x)
+#if defined(__GNUC__) // GCC and clang
#define ARROW_NORETURN __attribute__((noreturn))
#define ARROW_NOINLINE __attribute__((noinline))
#define ARROW_FORCE_INLINE __attribute__((always_inline))
+#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
+#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
#define ARROW_PREFETCH(addr) __builtin_prefetch(addr)
-#elif defined(_MSC_VER)
+#define ARROW_RESTRICT __restrict
+#if defined(__clang__) // clang-specific
+#define ARROW_COMPILER_ASSUME(expr) __builtin_assume(expr)
+#else
+#define ARROW_COMPILER_ASSUME(expr) \
+ if (!(expr)) { \
+ __builtin_unreachable(); \
Review Comment:
I got errors when using `clang` and having side-effects in the expression due to Arrow building with `-Werror` and many warnings enabled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1553193135
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,41 +36,62 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
//
-#if defined(__GNUC__)
-#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
-#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
+// [1] https://lobste.rs/s/uwgtkt/don_t_use_likely_unlikely_attributes#c_xi3wmc
+// [2] "Portable assumptions"
+// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf
+// [3] "Assertions Are Pessimistic, Assumptions Are Optimistic"
+// https://blog.regehr.org/archives/1096
+#define ARROW_UNUSED(x) (void)(x)
+#define ARROW_ARG_UNUSED(x)
+#if defined(__GNUC__) // GCC and clang
#define ARROW_NORETURN __attribute__((noreturn))
#define ARROW_NOINLINE __attribute__((noinline))
#define ARROW_FORCE_INLINE __attribute__((always_inline))
+#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
+#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
#define ARROW_PREFETCH(addr) __builtin_prefetch(addr)
-#elif defined(_MSC_VER)
+#define ARROW_RESTRICT __restrict
+#if defined(__clang__) // clang-specific
+#define ARROW_COMPILER_ASSUME(expr) __builtin_assume(expr)
+#else
+#define ARROW_COMPILER_ASSUME(expr) \
+ if (!(expr)) { \
+ __builtin_unreachable(); \
Review Comment:
Are we sure that this will never generate any additional code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1553190784
##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -181,14 +181,13 @@ class ARROW_EXPORT VarLengthListLikeBuilder : public ArrayBuilder {
if constexpr (is_list_view(TYPE::type_id)) {
sizes = array.GetValues<offset_type>(2);
}
+ const bool has_validity = array.buffers[0].data != NULLPTR;
const bool all_valid = !array.MayHaveLogicalNulls();
- const uint8_t* validity = array.HasValidityBitmap() ? array.buffers[0].data : NULLPTR;
ARROW_RETURN_NOT_OK(Reserve(length));
for (int64_t row = offset; row < offset + length; row++) {
- const bool is_valid =
- all_valid || (validity && bit_util::GetBit(validity, array.offset + row)) ||
- array.IsValid(row);
Review Comment:
I'm not sure why the original code was written like this, but if you care about performance (which seems the point of adding assumptions), then using `GetBit` into the validity bitmap should be slightly faster than going through the `IsValid` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1553194396
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,41 +36,62 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
//
-#if defined(__GNUC__)
-#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
-#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
+// [1] https://lobste.rs/s/uwgtkt/don_t_use_likely_unlikely_attributes#c_xi3wmc
+// [2] "Portable assumptions"
+// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf
+// [3] "Assertions Are Pessimistic, Assumptions Are Optimistic"
+// https://blog.regehr.org/archives/1096
+#define ARROW_UNUSED(x) (void)(x)
+#define ARROW_ARG_UNUSED(x)
+#if defined(__GNUC__) // GCC and clang
#define ARROW_NORETURN __attribute__((noreturn))
#define ARROW_NOINLINE __attribute__((noinline))
#define ARROW_FORCE_INLINE __attribute__((always_inline))
+#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
+#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
#define ARROW_PREFETCH(addr) __builtin_prefetch(addr)
-#elif defined(_MSC_VER)
+#define ARROW_RESTRICT __restrict
+#if defined(__clang__) // clang-specific
+#define ARROW_COMPILER_ASSUME(expr) __builtin_assume(expr)
Review Comment:
Can we try to keep these macros in alphabetical order?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #41021:
URL: https://github.com/apache/arrow/pull/41021#issuecomment-2045615472
FYI: I messed up a bit on the last force push and didn't include the alphabetizing macros commit. Nothing else was lost though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1553623249
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,41 +36,62 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
//
-#if defined(__GNUC__)
-#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
-#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
+// [1] https://lobste.rs/s/uwgtkt/don_t_use_likely_unlikely_attributes#c_xi3wmc
+// [2] "Portable assumptions"
+// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf
+// [3] "Assertions Are Pessimistic, Assumptions Are Optimistic"
+// https://blog.regehr.org/archives/1096
+#define ARROW_UNUSED(x) (void)(x)
+#define ARROW_ARG_UNUSED(x)
+#if defined(__GNUC__) // GCC and clang
#define ARROW_NORETURN __attribute__((noreturn))
#define ARROW_NOINLINE __attribute__((noinline))
#define ARROW_FORCE_INLINE __attribute__((always_inline))
+#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
+#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
#define ARROW_PREFETCH(addr) __builtin_prefetch(addr)
-#elif defined(_MSC_VER)
+#define ARROW_RESTRICT __restrict
+#if defined(__clang__) // clang-specific
+#define ARROW_COMPILER_ASSUME(expr) __builtin_assume(expr)
Review Comment:
I ordered them by safer to more dangerous/powerful. I can change to alphabetical :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1553637741
##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -181,14 +181,13 @@ class ARROW_EXPORT VarLengthListLikeBuilder : public ArrayBuilder {
if constexpr (is_list_view(TYPE::type_id)) {
sizes = array.GetValues<offset_type>(2);
}
+ const bool has_validity = array.buffers[0].data != NULLPTR;
const bool all_valid = !array.MayHaveLogicalNulls();
- const uint8_t* validity = array.HasValidityBitmap() ? array.buffers[0].data : NULLPTR;
ARROW_RETURN_NOT_OK(Reserve(length));
for (int64_t row = offset; row < offset + length; row++) {
- const bool is_valid =
- all_valid || (validity && bit_util::GetBit(validity, array.offset + row)) ||
- array.IsValid(row);
Review Comment:
`IsValid` gets inlined. The assumptions means when `IsValid` is expanded here, `if (this->buffers[0].data != NULLPTR) {` becomes `if (has_validity)`. As the user of the `IsValid` function I don't have to write code that manually inlines its contents, all I have to do is given enough information to the compiler and the inliner will do that for me.
```diff
inline bool IsValid(int64_t i) const {
- if (this->buffers[0].data != NULLPTR) {
+ if (has_validity) {
return bit_util::GetBit(this->buffers[0].data, i + this->offset);
} else {
const auto type = this->type->id();
if (type == Type::SPARSE_UNION) {
return !IsNullSparseUnion(i);
}
if (type == Type::DENSE_UNION) {
return !IsNullDenseUnion(i);
}
if (type == Type::RUN_END_ENCODED) {
return !IsNullRunEndEncoded(i);
}
return this->null_count != this->length;
}
}
```
The place with the biggest impact (in another PR) is where I specialize the entire loop based on the array having a null bitmap from the beggining:
```cpp
ARROW_COMPILER_ASSUME(array.buffers[0].data != nullptr);
if (array.IsValid(i)) {
...
```
`IsValid` gets inlined and much smaller:
```diff
inline bool IsValid(int64_t i) const {
- if (this->buffers[0].data != NULLPTR) {
return bit_util::GetBit(this->buffers[0].data, i + this->offset);
- } else {
- const auto type = this->type->id();
- if (type == Type::SPARSE_UNION) {
- return !IsNullSparseUnion(i);
- }
- if (type == Type::DENSE_UNION) {
- return !IsNullDenseUnion(i);
- }
- if (type == Type::RUN_END_ENCODED) {
- return !IsNullRunEndEncoded(i);
- }
- return this->null_count != this->length;
- }
- }
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1554657713
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,40 +36,63 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
//
+// [1] https://lobste.rs/s/uwgtkt/don_t_use_likely_unlikely_attributes#c_xi3wmc
+// [2] "Portable assumptions"
+// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf
+// [3] "Assertions Are Pessimistic, Assumptions Are Optimistic"
+// https://blog.regehr.org/archives/1096
+#define ARROW_UNUSED(x) (void)(x)
+#define ARROW_ARG_UNUSED(x)
#if defined(__GNUC__)
+// Both GCC and clang define __GNUC__
+#if defined(__clang__)
+// Only clang defines __clang__
+#define ARROW_COMPILER_ASSUME(expr) __builtin_assume(expr)
+#else
+#define ARROW_COMPILER_ASSUME(expr) \
+ if (!(expr)) { \
+ __builtin_unreachable(); \
+ }
+#endif
+#define ARROW_FORCE_INLINE __attribute__((always_inline))
+#define ARROW_NOINLINE __attribute__((noinline))
+#define ARROW_NORETURN __attribute__((noreturn))
#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
-#define ARROW_NORETURN __attribute__((noreturn))
-#define ARROW_NOINLINE __attribute__((noinline))
-#define ARROW_FORCE_INLINE __attribute__((always_inline))
#define ARROW_PREFETCH(addr) __builtin_prefetch(addr)
-#elif defined(_MSC_VER)
-#define ARROW_NORETURN __declspec(noreturn)
-#define ARROW_NOINLINE __declspec(noinline)
+#define ARROW_RESTRICT __restrict
+#elif defined(_MSC_VER) // MSVC
+#define ARROW_COMPILER_ASSUME(expr) __assume(expr)
#define ARROW_FORCE_INLINE __declspec(forceinline)
+#define ARROW_NOINLINE __declspec(noinline)
+#define ARROW_NORETURN __declspec(noreturn)
#define ARROW_PREDICT_FALSE(x) (x)
#define ARROW_PREDICT_TRUE(x) (x)
#define ARROW_PREFETCH(addr)
+#define ARROW_RESTRICT __restrict
Review Comment:
Because doing this is less confusing than creating a block for the things that work the same way on GNU and Microsoft compilers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1556031486
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,41 +36,62 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
//
-#if defined(__GNUC__)
-#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
-#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
+// [1] https://lobste.rs/s/uwgtkt/don_t_use_likely_unlikely_attributes#c_xi3wmc
+// [2] "Portable assumptions"
+// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf
+// [3] "Assertions Are Pessimistic, Assumptions Are Optimistic"
+// https://blog.regehr.org/archives/1096
+#define ARROW_UNUSED(x) (void)(x)
+#define ARROW_ARG_UNUSED(x)
+#if defined(__GNUC__) // GCC and clang
#define ARROW_NORETURN __attribute__((noreturn))
#define ARROW_NOINLINE __attribute__((noinline))
#define ARROW_FORCE_INLINE __attribute__((always_inline))
+#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
+#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
#define ARROW_PREFETCH(addr) __builtin_prefetch(addr)
-#elif defined(_MSC_VER)
+#define ARROW_RESTRICT __restrict
+#if defined(__clang__) // clang-specific
+#define ARROW_COMPILER_ASSUME(expr) __builtin_assume(expr)
+#else
+#define ARROW_COMPILER_ASSUME(expr) \
+ if (!(expr)) { \
+ __builtin_unreachable(); \
Review Comment:
Hmm, but this is a GCC-specific snippet.
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,40 +36,63 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
Review Comment:
Also @mapleFU 's link about possible pessimizations due to misuse of assumptions?
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,40 +36,63 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
//
+// [1] https://lobste.rs/s/uwgtkt/don_t_use_likely_unlikely_attributes#c_xi3wmc
+// [2] "Portable assumptions"
+// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf
+// [3] "Assertions Are Pessimistic, Assumptions Are Optimistic"
+// https://blog.regehr.org/archives/1096
+#define ARROW_UNUSED(x) (void)(x)
+#define ARROW_ARG_UNUSED(x)
#if defined(__GNUC__)
+// Both GCC and clang define __GNUC__
Review Comment:
Apparently, the Intel compiler also does :-) We can wait for bug reports, that said.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #41021:
URL: https://github.com/apache/arrow/pull/41021#issuecomment-2040252236
Thanks for introducing this. IMO assume is something like restrict. And it might not only brings optimization[1]. I'm glad to have this, but when we change the code maybe we need benchmark data
[1] https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1553653479
##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -181,14 +181,13 @@ class ARROW_EXPORT VarLengthListLikeBuilder : public ArrayBuilder {
if constexpr (is_list_view(TYPE::type_id)) {
sizes = array.GetValues<offset_type>(2);
}
+ const bool has_validity = array.buffers[0].data != NULLPTR;
const bool all_valid = !array.MayHaveLogicalNulls();
- const uint8_t* validity = array.HasValidityBitmap() ? array.buffers[0].data : NULLPTR;
ARROW_RETURN_NOT_OK(Reserve(length));
for (int64_t row = offset; row < offset + length; row++) {
- const bool is_valid =
- all_valid || (validity && bit_util::GetBit(validity, array.offset + row)) ||
- array.IsValid(row);
int64_t size = 0;
+ ARROW_COMPILER_ASSUME(has_validity == (array.buffers[0].data != NULLPTR));
Review Comment:
No, I explained here https://github.com/apache/arrow/pull/41021#discussion_r1553637741
This allows the compiler to assume `array.buffers[0].data != NULLPTR` is the same as `has_validity` so when `IsValid` is inlined, it replaces that memory-value check with a local-variable check (most likely stored in a register).
Assumption are things that (1) I prove to be be definitely true, and that (2) the compiler can trust as true and generate better code.
You can take a look on the links I posted in `macros.h` to understand assumptions better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #41021:
URL: https://github.com/apache/arrow/pull/41021#issuecomment-2039941641
I wonder would llvm / gcc have a pass or other things finding out that could it find the assertions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #41021:
URL: https://github.com/apache/arrow/pull/41021#issuecomment-2040265374
> And it might not only brings optimization[1]. I'm glad to have this, but when we change the code maybe we need benchmark data
>
> [1] https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609
Hmm, interesting. Thank you for the pointer. Also, assume might then have different drawbacks depending on the compiler, which is rather annoying.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1554520160
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,40 +36,63 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
//
+// [1] https://lobste.rs/s/uwgtkt/don_t_use_likely_unlikely_attributes#c_xi3wmc
+// [2] "Portable assumptions"
+// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf
+// [3] "Assertions Are Pessimistic, Assumptions Are Optimistic"
+// https://blog.regehr.org/archives/1096
+#define ARROW_UNUSED(x) (void)(x)
+#define ARROW_ARG_UNUSED(x)
#if defined(__GNUC__)
+// Both GCC and clang define __GNUC__
+#if defined(__clang__)
+// Only clang defines __clang__
+#define ARROW_COMPILER_ASSUME(expr) __builtin_assume(expr)
+#else
+#define ARROW_COMPILER_ASSUME(expr) \
+ if (!(expr)) { \
+ __builtin_unreachable(); \
+ }
+#endif
+#define ARROW_FORCE_INLINE __attribute__((always_inline))
+#define ARROW_NOINLINE __attribute__((noinline))
+#define ARROW_NORETURN __attribute__((noreturn))
#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
-#define ARROW_NORETURN __attribute__((noreturn))
-#define ARROW_NOINLINE __attribute__((noinline))
-#define ARROW_FORCE_INLINE __attribute__((always_inline))
#define ARROW_PREFETCH(addr) __builtin_prefetch(addr)
-#elif defined(_MSC_VER)
-#define ARROW_NORETURN __declspec(noreturn)
-#define ARROW_NOINLINE __declspec(noinline)
+#define ARROW_RESTRICT __restrict
+#elif defined(_MSC_VER) // MSVC
+#define ARROW_COMPILER_ASSUME(expr) __assume(expr)
#define ARROW_FORCE_INLINE __declspec(forceinline)
+#define ARROW_NOINLINE __declspec(noinline)
+#define ARROW_NORETURN __declspec(noreturn)
#define ARROW_PREDICT_FALSE(x) (x)
#define ARROW_PREDICT_TRUE(x) (x)
#define ARROW_PREFETCH(addr)
+#define ARROW_RESTRICT __restrict
Review Comment:
Why restrict is defined multiple times using same code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1556284805
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,41 +36,62 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
//
-#if defined(__GNUC__)
-#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
-#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
+// [1] https://lobste.rs/s/uwgtkt/don_t_use_likely_unlikely_attributes#c_xi3wmc
+// [2] "Portable assumptions"
+// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf
+// [3] "Assertions Are Pessimistic, Assumptions Are Optimistic"
+// https://blog.regehr.org/archives/1096
+#define ARROW_UNUSED(x) (void)(x)
+#define ARROW_ARG_UNUSED(x)
+#if defined(__GNUC__) // GCC and clang
#define ARROW_NORETURN __attribute__((noreturn))
#define ARROW_NOINLINE __attribute__((noinline))
#define ARROW_FORCE_INLINE __attribute__((always_inline))
+#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
+#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
#define ARROW_PREFETCH(addr) __builtin_prefetch(addr)
-#elif defined(_MSC_VER)
+#define ARROW_RESTRICT __restrict
+#if defined(__clang__) // clang-specific
+#define ARROW_COMPILER_ASSUME(expr) __builtin_assume(expr)
+#else
+#define ARROW_COMPILER_ASSUME(expr) \
+ if (!(expr)) { \
+ __builtin_unreachable(); \
Review Comment:
What I meant is that since we are also building with `clang` in CI, usages of `ARROW_COMPILER_ASSUME` that don't fail-to-compile on GCC will fail to compile on `clang`. I will extend a comment here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1553620775
##########
cpp/src/arrow/util/macros.h:
##########
@@ -36,41 +36,62 @@
TypeName& operator=(TypeName&&) = default
#endif
-#define ARROW_UNUSED(x) (void)(x)
-#define ARROW_ARG_UNUSED(x)
+// With ARROW_PREDICT_FALSE, GCC and clang can be told that a certain branch is
+// not likely to be taken (for instance, a CHECK failure), and use that information in
+// static analysis. Giving the compiler this information can affect the generated code
+// layout in the absence of better information (i.e. -fprofile-arcs). [1] explains how
+// this feature can be used to improve code generation. It was written as a positive
+// comment to a negative article about the use of these annotations.
//
-// GCC can be told that a certain branch is not likely to be taken (for
-// instance, a CHECK failure), and use that information in static analysis.
-// Giving it this information can help it optimize for the common case in
-// the absence of better information (ie. -fprofile-arcs).
+// ARROW_COMPILER_ASSUME allows the compiler to assume that a given expression
+// is true, without evaluating it, and to optimise based on this assumption [2].
+// If this condition is violated at runtime, the behavior is undefined. This can
+// be useful to generate both faster and smaller code in compute kernels. Different
+// optimisers are likely to react differently to this annotation. It should be used
+// with care [3] when we can prove by some means that the assumption is guaranteed
+// to always hold.
//
-#if defined(__GNUC__)
-#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
-#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
+// [1] https://lobste.rs/s/uwgtkt/don_t_use_likely_unlikely_attributes#c_xi3wmc
+// [2] "Portable assumptions"
+// https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1774r4.pdf
+// [3] "Assertions Are Pessimistic, Assumptions Are Optimistic"
+// https://blog.regehr.org/archives/1096
+#define ARROW_UNUSED(x) (void)(x)
+#define ARROW_ARG_UNUSED(x)
+#if defined(__GNUC__) // GCC and clang
#define ARROW_NORETURN __attribute__((noreturn))
#define ARROW_NOINLINE __attribute__((noinline))
#define ARROW_FORCE_INLINE __attribute__((always_inline))
+#define ARROW_PREDICT_FALSE(x) (__builtin_expect(!!(x), 0))
+#define ARROW_PREDICT_TRUE(x) (__builtin_expect(!!(x), 1))
#define ARROW_PREFETCH(addr) __builtin_prefetch(addr)
-#elif defined(_MSC_VER)
+#define ARROW_RESTRICT __restrict
+#if defined(__clang__) // clang-specific
+#define ARROW_COMPILER_ASSUME(expr) __builtin_assume(expr)
+#else
+#define ARROW_COMPILER_ASSUME(expr) \
+ if (!(expr)) { \
+ __builtin_unreachable(); \
Review Comment:
It won't because we build with clang as well and clang loudly complains about assumptions that have side-effects. One of the points of the C++23 standardization is to require that compilers don't generate code for the assumption expression itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #41021:
URL: https://github.com/apache/arrow/pull/41021#issuecomment-2040794686
I removed the use of the macro and force-pushed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Compiler assume [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41021:
URL: https://github.com/apache/arrow/pull/41021#issuecomment-2038496541
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41021:
URL: https://github.com/apache/arrow/pull/41021#issuecomment-2038498586
:warning: GitHub issue #41020 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41020: [C++] Introduce portable compiler assumptions [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #41021:
URL: https://github.com/apache/arrow/pull/41021#discussion_r1552930091
##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -181,14 +181,13 @@ class ARROW_EXPORT VarLengthListLikeBuilder : public ArrayBuilder {
if constexpr (is_list_view(TYPE::type_id)) {
sizes = array.GetValues<offset_type>(2);
}
+ const bool has_validity = array.buffers[0].data != NULLPTR;
const bool all_valid = !array.MayHaveLogicalNulls();
- const uint8_t* validity = array.HasValidityBitmap() ? array.buffers[0].data : NULLPTR;
ARROW_RETURN_NOT_OK(Reserve(length));
for (int64_t row = offset; row < offset + length; row++) {
- const bool is_valid =
- all_valid || (validity && bit_util::GetBit(validity, array.offset + row)) ||
- array.IsValid(row);
int64_t size = 0;
+ ARROW_COMPILER_ASSUME(has_validity == (array.buffers[0].data != NULLPTR));
Review Comment:
Why we do it here? To prove that `has_validity` ensures `data` is not null?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org