[abseil_cpp] Changed to build v20230125.0 #10560
Conversation
Added missing libabsl_flags product.
stemann
force-pushed
the
feature/abseil_cpp_20230125.0
branch
3 times, most recently
from
7485bb6 to
f8ce470
Compare
stemann
force-pushed
the
feature/abseil_cpp_20230125.0
branch
from
f8ce470 to
951319c
Compare
…for aarch64-*-march+armv8_2_crypto
A/abseil_cpp/build_tarballs.jl
Outdated
| # Avoid problems with `-march`, `-ffast-math` etc. | ||
| sed -i -e 's!set(CMAKE_C_COMPILER.*!set(CMAKE_C_COMPILER '${WORKSPACE}/srcdir/files/ccsafe')!' ${CMAKE_TARGET_TOOLCHAIN} | ||
| sed -i -e 's!set(CMAKE_CXX_COMPILER.*!set(CMAKE_CXX_COMPILER '${WORKSPACE}/srcdir/files/c++safe')!' ${CMAKE_TARGET_TOOLCHAIN} | ||
| if [[ "$target" == aarch64-* # Never apply `-march=armv8-a+crypto` -- even for aarch64-*-march+armv8_2_crypto |
There was a problem hiding this comment.
This is probably wrong.
It would likely be better to
- either not expand microarchs for aarch64 (as the compiled code is the same)
- or add a patch to the compiler wrappers to allow -march flags (for aarch64 only)
Suggestions?
There was a problem hiding this comment.
See also commit Excluded aarch64-apple-darwin from expand_microarchitectures
There was a problem hiding this comment.
Abseil doesn't seem to be a performance-critical library. It would make sense to not expand microarchitectures and just use the default that we're using for all other packages.
There was a problem hiding this comment.
True. The counterargument is that abseil seems to become a foundational dependency of many C++ packages, so it seems right to build it without (too many) patches - to have the "proper" thing at the base...
There was a problem hiding this comment.
If you distinguish between different microarchitectures then you need to make sure that this does not affect the ABI.
For example, on x86_64, if there is a function that accepts SIMD values, then the way in which they are passed depends on the microarchitecture settings. That forces all downstream packages to also be built in different versions, for different microarchitectures. Other packages (e.g. FFTW, OpenBLAS) have an ABI that is microarchitecture-independent, so this isn't a problem there. Of course, FFTW can then just make that distinction by itself at run time and thus you don't need to build different versions.
You could look at other package managers. For example, how does Debian or Ubuntu distribute Abseil? That's probably a safe choice to follow.
There was a problem hiding this comment.
On a related note (no. 1), should the -march=armv8-a+crypto be allowed for aarch64 in this specific case? (patching the compiler wrapper)
There was a problem hiding this comment.
On a related note (no. 2), should the BBB compiler wrappers complain (warn or error) about flags like -mfpu=neon, -maes, and -msse4.1?
There was a problem hiding this comment.
If there is a run-time check whether the hardware supports these CPU instructions then it's okay to compile the code, because it will not randomly crash at run time. If the code is used unconditionally then you should rely on the default architecture flags that BinaryBuilder is using, i.e. in this case you probably shouldn't be using any -march flags. Note that Julia uses compiler wrappers that inject extra compiler flags when you're building.
There was a problem hiding this comment.
Looks like there is runtime check: https://github.com/abseil/abseil-cpp/blob/20230125.0/absl/random/internal/randen_detect.h#L27
There was a problem hiding this comment.
I've added sed-based patching of compiler wrappers to explicitly allow compiler arg. -march=armv8-a+crypto
…h64-apple-darwin is only expanded to aarch64-apple-darwin-march+armv8_0, and not aarch64-apple-darwin-march+armv8_2_crypto as expected.
stemann
force-pushed
the
feature/abseil_cpp_20230125.0
branch
from
8f98528 to
7c5fd6e
Compare
…piler arg. `-march=armv8-a+crypto`
| # Allow setting "-march=armv8-a+crypto" | ||
| find $(dirname $(which $CC)) -type f \ | ||
| | xargs grep --files-with-matches 'BinaryBuilder: Cannot force an architecture via -march' \ | ||
| | xargs -n1 sed -i -E 's/^if \[\[ (" \$\{ARGS\[@\]\} " == \*"-march="\*) \]\]; then/if [[\n \1\n \&\& ! " \${ARGS[@]} " == *"-march=armv8-a+crypto"*\n]]; then/' |
There was a problem hiding this comment.
I'm a bit confused: aren't you patching the CMake file not to set -march=armv8-a+crypto?
There was a problem hiding this comment.
No, I’m just patching it for x86_64-apple-darwin - which is just to remedy some slightly opaque cmake configuration mess from upstream which causes the ARM64 flag to be applied for x86_64-apple-darwin.
There was a problem hiding this comment.
… and the X64 flags to be applied for aarch64-apple-darwin 🔀
There was a problem hiding this comment.
That sounds like an upstream bug?
There was a problem hiding this comment.
Yes, I believe so… but not sure if this version is open for patches (wrt. upstreaming).
There was a problem hiding this comment.
20230125 is past the two year due date, so I would not expect patches upstream to be handled.
https://abseil.io/about/releases
There was a problem hiding this comment.
Newer versions don't have this problem?
There was a problem hiding this comment.
I don’t know yet - my plan is to push a range of PRs to cover all abseil versions used by protobuf - this is the first:
There was a problem hiding this comment.
It definitely looks like they figured out something was bad around July 2024: abseil/abseil-cpp#1710
Added missing libabsl_flags product.
Also, added handling of microarch differences wrt. hardware AES flags by expanding architectures for aarch64, x86_64, and armv7l - instead of disabling optimizations via ccsafe (which included sed expression
's/ -march=[^ ]* / /g;s/ -maes / /g;s/ -msse4[.]1 / /g;s/ -Xarch_[^ ]* / /g')