Fix --disable-asm for newer assembly checks/code #13788

pull luke-jr wants to merge 3 commits into bitcoin:master from luke-jr:bugfix_asm_opt changing 1 files +11 −2
  1. luke-jr commented at 7:41 PM on July 28, 2018: member

    Fixes #13759

    Also inverts the help (so it shows --disable-asm like other enabled-by-default options, and initialises the flag variables.

  2. configure: Invert --enable-asm help string since default is now enabled d8ab8dc12d
  3. configure: Skip assembly support checks, when assembly is disabled afe0875577
  4. configure: Initialise assembly enable_* variables 4207c1b35c
  5. fanquake added the label Build system on Jul 29, 2018
  6. laanwj added this to the milestone 0.17.0 on Jul 30, 2018
  7. laanwj commented at 1:34 PM on July 30, 2018: member

    makes sense, utACK 4207c1b35c2e2ee1c9217cc7db3290a24c3b4b52

  8. achow101 commented at 12:51 AM on August 8, 2018: member

    utACK 4207c1b35c2e2ee1c9217cc7db3290a24c3b4b52

  9. ken2812221 commented at 3:45 AM on August 8, 2018: contributor

    ACK 4207c1b35c2e2ee1c9217cc7db3290a24c3b4b52

  10. sipa commented at 6:43 AM on August 8, 2018: member

    The variables enable_hwcrc, enable_sse41, enable_avx2, and enable_shani do not control inclusion of assembly code, but C++ with intrinsics. See my comment here: #13759 (comment).

    Overall my preference would be to remove that compilation flag, as I don't know under what conditions I'd suggest someone disable it.

  11. laanwj commented at 11:34 AM on August 8, 2018: member

    do not control inclusion of assembly code, but C++ with intrinsic

    I think many people interpret asm as "special instructions", independent of whether these are emitted though inline assembly or intrinsics.

    as I don't know under what conditions I'd suggest someone disable it.

    Also not sure on this. Clearly they should be disabled if the compiler doesn't know the intrinsics, but that is already done automatically.

  12. laanwj removed this from the milestone 0.17.0 on Aug 15, 2018
  13. laanwj commented at 2:22 PM on August 15, 2018: member

    Removing 0.17.0 milestone

  14. laanwj commented at 7:31 AM on November 23, 2018: member

    Overall my preference would be to remove that compilation flag, as I don't know under what conditions I'd suggest someone disable it.

    Recently there was someone with an edge case in the IRC channel with regard to assembly, where they needed to disable it to build. So I think it would be a bad idea to remove it.

    Edit: ah this was @MarcoFalke:

    2018-11-19 17:05:41     MarcoFalke      Anyone has an idea how to disable compilation of this: crypto/sha256_sse4.cpp:44:9: error: unknown token in expression
2018-11-19 17:05:46     MarcoFalke              "shl    $0x6,%2;"
2018-11-19 17:06:51     wumpus  MarcoFalke: --disable-asm?
2018-11-19 17:06:54     gmaxwell        MarcoFalke: there is a configure option to disable asm
2018-11-19 17:07:09     gmaxwell        lol wumpus' answer was faster an more informative.
2018-11-19 17:07:17     MarcoFalke      I thought it excluded those files, but let me try
2018-11-19 17:07:40     wumpus  although, ideally it *should* detect lack of support for assembly in the compiler in autoconf and do that automatically
2018-11-19 17:08:21     wumpus  there's even been talk of removing disable-asm but I think edge cases such as this make it useful to keep
2018-11-19 17:23:03     MarcoFalke      wumpus: gmaxwell: Thanks, that worked.
2018-11-19 17:23:54     gmaxwell        What strange compiler are you using where it cant compile crypto/sha256_sse4.cpp ?
2018-11-19 17:24:38     MarcoFalke      xenial on travis
2018-11-19 17:24:47     MarcoFalke      gcc5.something that is
2018-11-19 17:25:33     gmaxwell        I wonder if trais has done something weird, becuase that code should compile fine with gcc5.
2018-11-19 17:26:50     MarcoFalke      Its odd, I thought they used gce for the sudo-vms, but it would compile sse41.cpp on gce for me
  15. MarcoFalke added the label Needs gitian build on Apr 8, 2019
  16. MarcoFalke removed the label Needs gitian build on Apr 8, 2019
  17. MarcoFalke commented at 3:39 PM on April 8, 2019: member

    @luke-jr Is this still relevant after the recent discussion?

  18. luke-jr commented at 5:14 PM on April 8, 2019: member

    I don't know of any recent discussion, unless you mean the one quoted by @laanwj 6 months ago...

    I don't see why it would cease to be relevant.

  19. laanwj commented at 5:27 PM on April 8, 2019: member

    Maybe would make sense to discuss this at the IRC meeting

  20. practicalswift commented at 7:55 AM on April 9, 2019: contributor

    Concept ACK

    Please don't remove --disable-asm -- I often find the need for it when working with "non-vanilla compilation setups" such as when trying out research prototypes/research compiler extensions. A recent example was this EffectiveSan case https://github.com/GJDuck/EffectiveSan/issues/2#issuecomment-454262478 where --disable-asm was needed.

    More about EffectiveSan:

  21. laanwj commented at 8:40 AM on April 9, 2019: member

    Please don't remove --disable-asm -- I often find the need for it when working with "non-vanilla compilation setups" such as when trying out research prototypes/research compiler extensions.

    So does anyone agree with my idea of how disable-asm should be interpreted? It would, in your context, be the expected behavior too I think.

    I think many people interpret asm as "special instructions", independent of whether these are emitted though inline assembly or intrinsics.

  22. practicalswift commented at 9:01 AM on April 9, 2019: contributor

    I think many people interpret asm as "special instructions", independent of whether these are emitted though inline assembly or intrinsics. @laanwj Yes, agreed!

  23. luke-jr commented at 12:05 PM on April 9, 2019: member

    Yes, that's the intent of this change.

  24. MarcoFalke added the label Needs gitian build on Apr 9, 2019
  25. DrahtBot commented at 9:04 AM on April 10, 2019: member

    <!--a722867cd34abeea1fadc8d60700f111-->

    Gitian builds for commit a238fccce84bcd5544e88bdc37bf5b9b387d2164 (master):

    Gitian builds for commit c208fb7225e2389d2abd87ae172d55dcf76009ce (master and this pull):

  26. DrahtBot removed the label Needs gitian build on Apr 10, 2019
  27. sipa commented at 8:06 PM on April 11, 2019: member

    If the goal is appeasing platforms where autodetection or these optimizations fail for some other reason, it makes sense to have the option include the intrinsics.

    I don't care too strongly about the naming.

  28. practicalswift commented at 1:45 PM on April 26, 2019: contributor

    tACK 4207c1b35c2e2ee1c9217cc7db3290a24c3b4b52

    Now at two ACKs and two utACK:s -- perhaps ready for merge?

    That would help me since I frequently find the need for a --disable-asm working the way this PR makes it do. Currently I have to emulate this behaviour by manually patching configure.ac :-)

  29. MarcoFalke merged this on Apr 26, 2019
  30. MarcoFalke closed this on Apr 26, 2019
  31. MarcoFalke referenced this in commit b1e013e4fa on Apr 26, 2019
  32. sidhujag referenced this in commit 15d0fe0761 on Apr 27, 2019
  33. codablock referenced this in commit b5ea6638fc on Oct 1, 2019
  34. codablock referenced this in commit 4bfc6ab303 on Oct 1, 2019
  35. barrystyle referenced this in commit a2b41857d1 on Jan 22, 2020
  36. DrahtBot locked this on Dec 16, 2021
Contributors
luke-jrlaanwjachow101ken2812221sipaMarcoFalkepracticalswiftDrahtBot
Labels
Build system
Linked (view graph)
#13759 Assembly optimisations are compiled even with --disable-asm
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-14 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me