Enable LTO on the Linux Clang build on CI #3491
Conversation
| @@ -137,7 +137,7 @@ ENDFOREACH() | |||
|
|
|||
| option(BYN_ENABLE_LTO "Build with LTO" Off) | |||
| if(BYN_ENABLE_LTO) | |||
| if(NOT CMAKE_CXX_COMPILER_ID STREQUAL "Clang") | |||
| if(NOT CMAKE_CXX_COMPILER_ID MATCHES "Clang") | |||
There was a problem hiding this comment.
is this needed? docs say it allows regexes, but the right side isn't one?
There was a problem hiding this comment.
Apple's clang has the compiler ID AppleClang which MATCHES clang but isn't STREQUAL. Using MATCHES with bare strings seems pretty common in CMake files (it definitely is in LLVM's)
.github/workflows/ci.yml
Outdated
| @@ -70,7 +70,7 @@ jobs: | |||
| if: matrix.os == 'ubuntu-latest' | |||
|
|
|||
| - name: cmake (macos) | |||
| run: cmake -S . -B out -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=out/install | |||
| run: cmake -S . -B out -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=out/install -DBYN_ENABLE_LTO=ON | |||
There was a problem hiding this comment.
I'd suggest not changing the mac build since it's what we ship to users atm.
(The linux "clang" build sgtm to change to LTO, since we do have a bunch of other non-LTO builds on linux anyhow.)
There was a problem hiding this comment.
OK yeah, that's sort of what I had in mind for now too.
I think we do want to ship this to users eventually though if it doesn't end up being too much trouble.
There was a problem hiding this comment.
Agreed, yeah, would be good to ship this. (And switch testing then - but I don't feel strongly about the meantime either.)
There was a problem hiding this comment.
Speaking of, I noticed that we still force assertions on. Are we close to no longer requiring that?
There was a problem hiding this comment.
(also our current command line puts a -O2 and overrides the default -O3)
There was a problem hiding this comment.
I think the status is not changed from #3065 - we were thinking of testing with assertions but shipping to users without, which would need more work. LTO is probably a bigger deal though than assertions, from your data.
There was a problem hiding this comment.
I found it to be fairly evenly split between them, actually. Although those numbers were probably dominated by LLVM rather than Binaryen. For emscripten-releases I think we do want to test with assertions for LLVM as well. Not sure how much difference it makes on the standalone releases.
dschuff
changed the title
No description provided.