build: use `-muse-unaligned-vector-move` for Windows builds

fanquake commented at 4:25 pm on July 25, 2023: member

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:

0x86: Add -muse-unaligned-vector-move to assembler
1
2Unaligned load/store instructions on aligned memory or register are as
3fast as aligned load/store instructions on modern Intel processors.  Add
4a command-line option, -muse-unaligned-vector-move, to x86 assembler to
5encode encode aligned vector load/store instructions as unaligned
6vector load/store instructions.

DrahtBot commented at 4:25 pm on July 25, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot renamed this:
~~[WIP] guix: use `-muse-unaligned-vector-move` for Windows builds~~
[WIP] guix: use `-muse-unaligned-vector-move` for Windows builds
on Jul 25, 2023

fanquake commented at 4:25 pm on July 25, 2023: member

cc @theuni

maflcko added the label DrahtBot Guix build requested on Jul 25, 2023

DrahtBot added the label CI failed on Jul 25, 2023

theuni commented at 8:57 pm on July 25, 2023: member

Concept ACK. The patch we’ve taken from Debian is described as:

replacing the aligned instruction with the unaligned one the hacky patch below got created, just replacing vmovapd by vmovupd. Not considering any side effects and maybe other instructions with alignment requirements.

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?

theuni commented at 8:58 pm on July 25, 2023: member

Any reason not to add it to our configure directly rather than guix?

DrahtBot commented at 6:26 am on July 26, 2023: contributor

Guix builds

File	commit e35fb7bc48d360585b80d0c7f89ac5087c1d405e(master)	commit e590bc247ff4b0f8a112c7c7bb79c4b53656d435(master and this pull)
SHA256SUMS.part	`85ece518a8ab8bb8...`	`d0a52d153d899e55...`
*-aarch64-linux-gnu-debug.tar.gz	`ec06911cbfa43bcb...`	`f5d30fe5bc686d43...`
*-aarch64-linux-gnu.tar.gz	`9cf832869ea5eec6...`	`74547f380b8ebb65...`
*-arm-linux-gnueabihf-debug.tar.gz	`ce0894bf1cb47527...`	`a9c25bd6f128ba38...`
*-arm-linux-gnueabihf.tar.gz	`3260f94e70f77d08...`	`876aef59b552b1ff...`
*-arm64-apple-darwin-unsigned.dmg	`eb924e766c7c030b...`
*-arm64-apple-darwin-unsigned.tar.gz	`1aaa58cb290326c5...`
*-arm64-apple-darwin.tar.gz	`ce1ee32821b949a9...`
*-powerpc64-linux-gnu-debug.tar.gz	`e0072e370893cd22...`
*-powerpc64-linux-gnu.tar.gz	`69b80b8ce06f5c8e...`
*-powerpc64le-linux-gnu-debug.tar.gz	`36160eed83f577ac...`	`374aeccc6c19c180...`
*-powerpc64le-linux-gnu.tar.gz	`ea7524ac63244d44...`	`2b71c4aadb7837fe...`
*-riscv64-linux-gnu-debug.tar.gz	`278b77020585377a...`	`691bdddee95f8ba2...`
*-riscv64-linux-gnu.tar.gz	`fe51f12924ee6954...`	`fd2665f478718a25...`
*-x86_64-apple-darwin-unsigned.dmg	`3c96ead660b80011...`
*-x86_64-apple-darwin-unsigned.tar.gz	`5d1a5aca6189a136...`
*-x86_64-apple-darwin.tar.gz	`cdcf1d209ecadba4...`
*-x86_64-linux-gnu-debug.tar.gz	`6679dd3d862109bc...`	`4ec1c2b4508b2d2a...`
*-x86_64-linux-gnu.tar.gz	`2de809754a1009c5...`	`f550c8f278e0a492...`
*.tar.gz	`33df1d211b914d3a...`	`c7cfd1278124b629...`
guix_build.log	`551a5b95262a29c5...`	`799518d1848f5118...`
guix_build.log.diff		`aef8e939042efead...`

DrahtBot removed the label DrahtBot Guix build requested on Jul 26, 2023

fanquake force-pushed on Jul 27, 2023

DrahtBot removed the label CI failed on Jul 27, 2023

fanquake force-pushed on Aug 2, 2023

hebasto commented at 2:18 pm on August 2, 2023: member

Tested Guix build on Windows:

0C:\Users\hebasto\Desktop\pr28151\bitcoin-6c3f6a9d015c-win64\bitcoin-6c3f6a9d015c>bin\bitcoind.exe -signet
1*** stack smashing detected ***:  terminated

DrahtBot added the label CI failed on Aug 2, 2023

fanquake commented at 4:26 pm on August 3, 2023: member

Any reason not to add it to our configure directly rather than guix?

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 bitcoind.exe), while also retaining our GCC patching in Guix. Anything else would potentially leave us with unwanted code from dependencies/the rest of the toolchain.

theuni commented at 3:45 pm on August 4, 2023: member

Any reason not to add it to our configure directly rather than guix?

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 bitcoind.exe), while also retaining our GCC patching in Guix.

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.

fanquake commented at 12:43 pm on August 15, 2023: member

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).

DrahtBot added the label Needs rebase on Aug 22, 2023

fanquake force-pushed on Aug 23, 2023

fanquake renamed this:
~~[WIP] guix: use `-muse-unaligned-vector-move` for Windows builds~~
build: use `-muse-unaligned-vector-move` for Windows builds
on Aug 23, 2023

DrahtBot removed the label Needs rebase on Aug 23, 2023

DrahtBot removed the label CI failed on Aug 23, 2023

DrahtBot added the label Build system on Aug 23, 2023

fanquake force-pushed on Aug 26, 2023

build: use -muse-unaligned-vector-move for Windows

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 #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.

96f2cf8d2c

fanquake force-pushed on Aug 30, 2023

theuni commented at 6:44 pm on August 30, 2023: member

ACK 96f2cf8d2c3e0fba2a39dabd991dee69124cc79d.

I verified locally that trying to use an assembler that doesn’t understand the new option results in a compile failure:

0 $ g++ -I. -std=c++17 -c test.cpp -o test.o -Wa,-muse-unaligned-vector-move || echo failure
1as: unrecognized option '-muse-unaligned-vector-move'
2failure

I 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 sha-ni code as well, for example.

fanquake marked this as ready for review on Sep 5, 2023

fanquake commented at 12:36 pm on September 5, 2023: member

TODO: Note that there is usage of vmovapd in this branch and the current release binaries. Need to follow up.

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 ecab855838fa4de4c6d8c11e69037477d6047790. In any case, opened #28413, because we could add a test for the non-existence, while we continue to patch this behaviour out.

fanquake merged this on Sep 5, 2023

fanquake closed this on Sep 5, 2023

fanquake deleted the branch on Sep 5, 2023

Frank-GER referenced this in commit 703929bc00 on Sep 8, 2023

bitcoin locked this on Sep 4, 2024

build: use -muse-unaligned-vector-move for Windows builds #28151

Reviews

Conflicts

Guix builds

build: use `-muse-unaligned-vector-move` for Windows builds #28151