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.

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

build: Rename --enable-experimental-asm to --enable-asm and enable by default #11176

Merged
merged 2 commits into from Sep 5, 2017

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Aug 28, 2017

Now that 0.15 is branched off, enable assembler SHA256 optimizations by default, but still allow disabling them, for example if something goes wrong with auto-detection on a platform.

Also add mention of the use of asm in the configure summary.

… default

Now that 0.15 is branched off, enable assembler SHA256 optimizations by default.
@laanwj laanwj force-pushed the 2017_08_non_experimental_asm branch from f79326e to ce5381e Compare August 28, 2017 09:06
@promag
Copy link
Member

promag commented Aug 28, 2017

Should the flag mention SHA256 like --enable-asm-sha256?

Either way LGTM.

@laanwj
Copy link
Member Author

laanwj commented Aug 28, 2017

Should the flag mention SHA256 like --enable-asm-sha256?

No, I think we should have a general asm flag, build-side, as this does. This is similar to OpenSSL's flag to use assembly or not. Something else creates too many combinations to test, and I don't see why it would be useful.
(besides testing, but run-time selection is much more useful for that)

@promag
Copy link
Member

promag commented Aug 28, 2017

Ok, just had to ask.

utACK 538cc0c.

@kallewoof
Copy link
Member

If this is meant for 0.15, the release notes need to be updated. They currently read

SHA256 hashing has been optimized for architectures supporting SSE 4 (See PR 10182). SHA256 is around 50% faster on supported hardware, which results in around 5% faster IBD and block validation. In version 0.15, SHA256 hardware optimization is disabled in release builds by default, but can be enabled by using --enable-experimental-asm when building.

@laanwj
Copy link
Member Author

laanwj commented Aug 28, 2017

No, this is not meant for 0.15, certainly not for 0.15.0

Now that 0.15 is branched off, enable assembler SHA256 optimizations by default

@kallewoof
Copy link
Member

Sorry about that. I should've read the PR message more closely.

@sipa
Copy link
Member

sipa commented Aug 28, 2017

@laanwj There are plenty more versions of SHA256 to add (for starters, a RORX version, a SHA-NI version, but later perhaps a 4-way parallel SSE4 or 8-way parallel AVX2 version). Do we all want to include them under "--enable-asm", or will they first go into a reinstated "--enable-experimental-asm" before being promoted?

@laanwj
Copy link
Member Author

laanwj commented Aug 28, 2017

Do we all want to include them under "--enable-asm" or will they first go into a reinstated "--enable-experimental-asm" before being promoted?

Both are valid options, we should probably decide on a case by case basis - depending on how much we trust the code, how long it can be in master until the release.

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

utACK

@fanquake
Copy link
Member

fanquake commented Sep 2, 2017

utACK 538cc0c

@laanwj laanwj merged commit 538cc0c into bitcoin:master Sep 5, 2017
laanwj added a commit that referenced this pull request Sep 5, 2017
… and enable by default

538cc0c build: Mention use of asm in summary (Wladimir J. van der Laan)
ce5381e build: Rename --enable-experimental-asm to --enable-asm and enable by default (Wladimir J. van der Laan)

Pull request description:

  Now that 0.15 is branched off, enable assembler SHA256 optimizations by default, but still allow disabling them, for example if something goes wrong with auto-detection on a platform.

  Also add mention of the use of asm in the configure summary.

Tree-SHA512: cd20c497f65edd6b1e8b2cc3dfe82be11fcf4777543c830ccdec6c10f25eab4576b0f2953f3957736d7e04deaa4efca777aa84b12bb1cecb40c258e86c120ec8
sickpig referenced this pull request in sickpig/BitcoinUnlimited Sep 13, 2017
… and enable by default

538cc0ca8 build: Mention use of asm in summary (Wladimir J. van der Laan)
ce5381e7f build: Rename --enable-experimental-asm to --enable-asm and enable by default (Wladimir J. van der Laan)

Pull request description:

  Now that 0.15 is branched off, enable assembler SHA256 optimizations by default, but still allow disabling them, for example if something goes wrong with auto-detection on a platform.

  Also add mention of the use of asm in the configure summary.

Tree-SHA512: cd20c497f65edd6b1e8b2cc3dfe82be11fcf4777543c830ccdec6c10f25eab4576b0f2953f3957736d7e04deaa4efca777aa84b12bb1cecb40c258e86c120ec8
gandrewstone referenced this pull request in gandrewstone/BitcoinUnlimited Sep 13, 2017
Merge Port's core #11176: build: Rename --enable-experimental-asm to --enable-asm…
sickpig referenced this pull request in sickpig/BitcoinUnlimited Oct 13, 2017
…ble-asm and enable by default

538cc0ca8 build: Mention use of asm in summary (Wladimir J. van der Laan)
ce5381e7f build: Rename --enable-experimental-asm to --enable-asm and enable by default (Wladimir J. van der Laan)

Pull request description:

  Now that 0.15 is branched off, enable assembler SHA256 optimizations by default, but still allow disabling them, for example if something goes wrong with auto-detection on a platform.

  Also add mention of the use of asm in the configure summary.

Tree-SHA512: cd20c497f65edd6b1e8b2cc3dfe82be11fcf4777543c830ccdec6c10f25eab4576b0f2953f3957736d7e04deaa4efca777aa84b12bb1cecb40c258e86c120ec8
gandrewstone referenced this pull request in BitcoinUnlimited/BitcoinUnlimited Oct 19, 2017
Backport of Core #10821 and #11176
@droark
Copy link
Contributor

droark commented Dec 12, 2017

Posthumous review comment: Is USE_ASM meant to be a catch-all for any other assembly optimizations added in the future, or is it solely for the SHA256 optimizations? I ask because the language in the changes is a little ambiguous (IMO). I'd like to fix that. I just don't want to step on toes first.

Thanks.

codablock pushed a commit to codablock/dash that referenced this pull request Oct 1, 2019
…ble-asm and enable by default

538cc0c build: Mention use of asm in summary (Wladimir J. van der Laan)
ce5381e build: Rename --enable-experimental-asm to --enable-asm and enable by default (Wladimir J. van der Laan)

Pull request description:

  Now that 0.15 is branched off, enable assembler SHA256 optimizations by default, but still allow disabling them, for example if something goes wrong with auto-detection on a platform.

  Also add mention of the use of asm in the configure summary.

Tree-SHA512: cd20c497f65edd6b1e8b2cc3dfe82be11fcf4777543c830ccdec6c10f25eab4576b0f2953f3957736d7e04deaa4efca777aa84b12bb1cecb40c258e86c120ec8
codablock pushed a commit to codablock/dash that referenced this pull request Oct 1, 2019
…ble-asm and enable by default

538cc0c build: Mention use of asm in summary (Wladimir J. van der Laan)
ce5381e build: Rename --enable-experimental-asm to --enable-asm and enable by default (Wladimir J. van der Laan)

Pull request description:

  Now that 0.15 is branched off, enable assembler SHA256 optimizations by default, but still allow disabling them, for example if something goes wrong with auto-detection on a platform.

  Also add mention of the use of asm in the configure summary.

Tree-SHA512: cd20c497f65edd6b1e8b2cc3dfe82be11fcf4777543c830ccdec6c10f25eab4576b0f2953f3957736d7e04deaa4efca777aa84b12bb1cecb40c258e86c120ec8
barrystyle pushed a commit to PACGlobalOfficial/PAC that referenced this pull request Jan 22, 2020
…ble-asm and enable by default

538cc0c build: Mention use of asm in summary (Wladimir J. van der Laan)
ce5381e build: Rename --enable-experimental-asm to --enable-asm and enable by default (Wladimir J. van der Laan)

Pull request description:

  Now that 0.15 is branched off, enable assembler SHA256 optimizations by default, but still allow disabling them, for example if something goes wrong with auto-detection on a platform.

  Also add mention of the use of asm in the configure summary.

Tree-SHA512: cd20c497f65edd6b1e8b2cc3dfe82be11fcf4777543c830ccdec6c10f25eab4576b0f2953f3957736d7e04deaa4efca777aa84b12bb1cecb40c258e86c120ec8
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants