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 bolt #276

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

build bolt #276

wants to merge 10 commits into from

Conversation

h-vetinari
Copy link
Member

@h-vetinari h-vetinari commented Jul 10, 2024

Building bolt as standalone is not supported upstream (c.f. conda-forge/staged-recipes#22204), the README recommends building it as part of llvmdev. Let's try that here...

Xref #147

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@h-vetinari
Copy link
Member Author

@conda-forge/llvmdev @conda-forge/core
This is another LLVM component that we've been wanting to build for a long time (#147). BOLT is getting more popular as it can squeeze a further couple percentage points of performance out of otherwise already maximally optimized builds (e.g. 5% for the kernel; CPython gained support as well, etc.)

I tried adding bolt a couple of times in conda-forge/staged-recipes#22204, but standalone builds are unsupported, and the recommended way is to build this as part of building ../llvm, which is what this PR does.

The main downside of doing this in this feedstock is that this would further blow up the build time here, and we're already prone to hitting timeouts on the slower agents. Alternatively, we could create a separate feedstock that rebuilds ../llvm, but simply doesn't repackage the bits that belong to llvmdev.

The latter would be my preferred approach for various reasons, and I can easily transfer the changes from here to conda-forge/staged-recipes#22204. For now I'm still using this feedstock because it nicely shows the incremental changes, without getting lost in the noise if copied to staged-recipes.

Would appreciate your thoughts!

@xhochy
Copy link
Member

xhochy commented Jul 12, 2024

Alternatively, we could create a separate feedstock that rebuilds ../llvm, but simply doesn't repackage the bits that belong to llvmdev.

That sounds like something that can easily get out of sync. If we really need to do this because of build times, I would be Ok with it, but would very much prefer to avoid it.

@h-vetinari
Copy link
Member Author

That sounds like something that can easily get out of sync.

Both feedstocks would get built from the same version/commit. Or do you mean differences arising from mismatched build options?

@xhochy
Copy link
Member

xhochy commented Jul 12, 2024

I'm mostly worried about diverging build scripts.

@h-vetinari
Copy link
Member Author

@isuruf, any thoughts on this?

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

@h-vetinari
Copy link
Member Author

I'm mostly concerned about the build timeouts on windows; the last run took 3 attempts to complete, if that happens similarly after merging, we're looking at ~36h minimum to unblock the central feedstock upon which the rest LLVM stack builds for any new version (and that's assuming timely restarts and no other build failures to take care of).

If we decide to build bolt on this feedstock, then at least I'd want to be able to skip the bolt builds for a new version.

@isuruf
Copy link
Member

isuruf commented Jul 23, 2024

Then it's better to build as a separate feedstock. It should be possible to reduce the build time by building only the x86_64 and aarch64 backends as bolt only supports those.

@h-vetinari
Copy link
Member Author

We could also drop the check-llvm and/or lit tests on windows - I don't think they ever revealed a failure, much less a platform-specific one (since we'd keep running them on linux).

2024-07-23T13:39:54.4493894Z + ninja -j2 check-llvm
[...]
2024-07-23T14:25:44.6509014Z [663/664] Linking CXX executable unittests/tools/llvm-mca/LLVMMCATests
2024-07-23T14:25:44.6510711Z [663/664] Running the LLVM regression tests
2024-07-23T14:25:49.4332567Z -- Testing: 50606 tests, 2 workers --
2024-07-23T15:22:52.1838981Z Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
2024-07-23T15:22:52.6287365Z 
2024-07-23T15:22:52.6289495Z Testing Time: 3422.77s
2024-07-23T15:22:52.6290161Z 
2024-07-23T15:22:52.6290604Z Total Discovered Tests: 58184
2024-07-23T15:22:52.6291116Z   Skipped          :    15 (0.03%)
2024-07-23T15:22:52.6291726Z   Unsupported      :  1924 (3.31%)
2024-07-23T15:22:52.6292148Z   Passed           : 56089 (96.40%)
2024-07-23T15:22:52.6292615Z   Expectedly Failed:   156 (0.27%)
2024-07-23T15:22:53.2660625Z + cd ../llvm/test
2024-07-23T15:22:53.2662224Z + python ../../build/bin/llvm-lit -vv Transforms ExecutionEngine Analysis CodeGen/X86
[...]
2024-07-23T15:38:03.8619718Z Testing Time: 909.35s
2024-07-23T15:38:03.8620089Z 
2024-07-23T15:38:03.8620516Z Total Discovered Tests: 15438
2024-07-23T15:38:03.8620948Z   Unsupported      :   612 (3.96%)
2024-07-23T15:38:03.8621389Z   Passed           : 14779 (95.73%)
2024-07-23T15:38:03.8622107Z   Expectedly Failed:    47 (0.30%)

That would save us up to ~2h runtime on a slow windows agent (using the logs from the latest run that failed here), which would put us firmly back into the green again. We've already skipped the tests on osx for llvm 18 due to timeouts (cebf7e9).

Middleground would be to still keep running the lit tests on osx/win, but don't bother with check-llvm anymore. That would still be more than enough for green builds, and it would keep the recipe together as desired by @xhochy. WDYT?

@h-vetinari
Copy link
Member Author

h-vetinari commented Jul 23, 2024

OK, nevermind, I was so convinced that the job that timed out job was windows that I didn't see that I was looking at linux logs for ce3e17c 🙈

@h-vetinari
Copy link
Member Author

On windows now, the lit tests took:

2024-07-23T13:50:20.1734363Z (%BUILD_PREFIX%) %SRC_DIR%\llvm\test>python ..\..\build\bin\llvm-lit.py -vv Transforms ExecutionEngine Analysis CodeGen/X86 
[...]
2024-07-23T14:17:10.7091534Z Testing Time: 1607.39s
2024-07-23T14:17:10.7114067Z 
2024-07-23T14:17:10.8187639Z Total Discovered Tests: 15438
2024-07-23T14:17:10.8199971Z   Unsupported      :   660 (4.28%)
2024-07-23T14:17:10.8234575Z   Passed           : 14716 (95.32%)
2024-07-23T14:17:10.8296604Z   Expectedly Failed:    62 (0.40%)

In other words, roughly half an hour we could save

don't run check-llvm on osx but keep lit checks
on windows, we drop the lit tests too
@h-vetinari
Copy link
Member Author

h-vetinari commented Jul 24, 2024

It should be possible to reduce the build time by building only the x86_64 and aarch64 backends as bolt only supports those.

It seems supports risc-v as well...

[3213/3667] Linking CXX static library lib/libLLVMBOLTTargetAArch64.a
[3214/3667] Linking CXX static library lib/libLLVMBOLTTargetX86.a
[3215/3667] Linking CXX static library lib/libLLVMBOLTTargetRISCV.a

The problem is that the variable for target support is sounds llvm-wide and not bolt-specific (see README):

-DLLVM_TARGETS_TO_BUILD="X86;AArch64"

AFAIU we shouldn't touch that on linux (assuming we're still on llvmdev), or we'd have trouble cross-compiling to ppc? But we could switch it off on osx/win.

@isuruf
Copy link
Member

isuruf commented Jul 24, 2024

The problem is that the variable for target support is sounds llvm-wide and not bolt-specific (see README):

I meant, we can do it in a separate feedstock with reduced architectures. We shouldn't reduce architectures here.

@isuruf
Copy link
Member

isuruf commented Jul 24, 2024

Or we wait for llvm/llvm-project#97130. Either way, let's not package bolt in this feedstock.

@h-vetinari
Copy link
Member Author

Or we wait for llvm/llvm-project#97130. Either way, let's not package bolt in this feedstock.

Thanks for the reference. OK, I'll update conda-forge/staged-recipes#22204

@h-vetinari h-vetinari marked this pull request as draft July 24, 2024 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants