build: use -muse-unaligned-vector-move for Windows builds
#28151
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
DrahtBot
changed the title
-muse-unaligned-vector-move for Windows builds -muse-unaligned-vector-move for Windows builds
|
cc @theuni |
|
Concept ACK. The patch we've taken from Debian is described as:
Which appears to be what the new assembler option does as well. Using the supported switch is much better than the hacky patch. What's up with the single aligned case though? Is that maybe coming from somewhere where we explicitly specify alignment? |
|
Any reason not to add it to our configure directly rather than guix? |
Guix builds
|
fanquake
force-pushed
the
win_muse_unaligned_vector_move
branch
from
970b689
to
4480119
Compare
fanquake
force-pushed
the
win_muse_unaligned_vector_move
branch
from
4480119
to
6c3f6a9
Compare
|
Tested Guix build on Windows: |
I think that's actually all we can do here, adding this (to configure) as a best-effort for anyone using an unpatched compiler (where it'd be most beneficial when building |
Are you sure those two aren't mutually exclusive? If so, yeah, that sounds reasonable. Though I think we should make an effort to produce working binaries without the patch first, just to make sure the feature is actually working as intended. Then re-add the patch for belt-and-suspenders if possible. Thinking this through further, surely depends needs to be built with the flag as well? And possibly worse if toolchain objects need it too, but I guess depends might be enough? Edit: Ok, right, this is avx-only. Ignore most of the above. |
Yea. It seems hard to (generally) produce working Windows binaries outside of Guix, because I assume most Windows toolchains are broken with the same bug (unless they are all being patched similar to Debian/Ubuntu). So the best we can do is retain our GCC patching, so the entire toolchain/libs/bins are built correctly, and then in configure, we could pass the assembler flags as a best-effort, to try and produce working binaries when building outside of Guix (and I don't think addition of the flags to the Guix build (from configure) would be an issue, as it would only be swapping out instructions that shouldn't be appearing in any case). |
fanquake
force-pushed
the
win_muse_unaligned_vector_move
branch
from
6c3f6a9
to
83d8592
Compare
fanquake
changed the title
-muse-unaligned-vector-move for Windows builds-muse-unaligned-vector-move for Windows builds
fanquake
force-pushed
the
win_muse_unaligned_vector_move
branch
from
83d8592
to
77f33df
Compare
We currently work around a longstanding GCC issue with aligned vector instructions, in our release builds, by patching the behaviour we want into GCC (see discussion in bitcoin#24736). A new option now exists in the binutils assembler, `-muse-unaligned-vector-move`, which should also achieve the behaviour we want (at least for our code). This was added in the 2.38 release, see https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=c8480b58e1968f209b6365af7422678f348222c2. ```bash x86: Add -muse-unaligned-vector-move to assembler Unaligned load/store instructions on aligned memory or register are as fast as aligned load/store instructions on modern Intel processors. Add a command-line option, -muse-unaligned-vector-move, to x86 assembler to encode encode aligned vector load/store instructions as unaligned vector load/store instructions. ``` Even if we introduce this option into our build system, we'll have to maintain our GCC patching, as we want all code that ends up in the binary, to avoid these instructions. However, there may be some value in adding the option, as it could be an improvement for someone building (bitcoind.exe) with an unpatched compiler.
fanquake
force-pushed
the
win_muse_unaligned_vector_move
branch
from
77f33df
to
96f2cf8
Compare
|
ACK 96f2cf8. I verified locally that trying to use an assembler that doesn't understand the new option results in a compile failure: $ g++ -I. -std=c++17 -c test.cpp -o test.o -Wa,-muse-unaligned-vector-move || echo failure
as: unrecognized option '-muse-unaligned-vector-move'
failureI also considered suggesting moving this to avx flags, but I guess it doesn't hurt to use it everywhere. It may end up having an effect on |
I might have either imagined this, or been looking at binaries that I'd broken, as I cannot seem to find any instructions in a Guix built ecab855. In any case, opened #28413, because we could add a test for the non-existence, while we continue to patch this behaviour out. |
…dows builds 96f2cf8 build: use -muse-unaligned-vector-move for Windows (fanquake) Pull request description: We currently work around a longstanding GCC issue with aligned vector instructions, by patching the behaviour we want into GCC (see discussion in bitcoin#24736). Possibly in response to the GCC thread (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54412#c40), a new option was [introduced into the binutils assembler](https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=c8480b58e1968f209b6365af7422678f348222c2) with the 2.38 release: ``` x86: Add -muse-unaligned-vector-move to assembler Unaligned load/store instructions on aligned memory or register are as fast as aligned load/store instructions on modern Intel processors. Add a command-line option, -muse-unaligned-vector-move, to x86 assembler to encode encode aligned vector load/store instructions as unaligned vector load/store instructions. ``` ACKs for top commit: theuni: ACK 96f2cf8. Tree-SHA512: f5aaa125922bb17bbb51454103b3394d293184214b0dea554c36c2f130488a3ede2f39678044ec846fa0fdf4cd441d4227f4565c29d380f5a73b50bf6f3b9a67
We currently work around a longstanding GCC issue with aligned vector instructions, by patching the behaviour we want into GCC (see discussion in #24736). Possibly in response to the GCC thread (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54412#c40), a new option was introduced into the binutils assembler with the 2.38 release: