Dist Enzyme for macOS with unversioned LLVM dylib#151063
Dist Enzyme for macOS with unversioned LLVM dylib#151063sgasho wants to merge 6 commits intorust-lang:mainfrom
Conversation
|
@bors delegate try |
|
Unknown command "delegate". Run |
|
Hmm, not sure about the exact syntax, but delegate itself should have been recognized, maybe not ported to the new bots yet? I'll let Jakub handle it. @bors try jobs=dist-aarch64-apple |
This comment has been minimized.
This comment has been minimized.
Add dist step for enzyme, including aarch64-apple-darwin try-job: dist-aarch64-apple
|
@bors delegate=try |
|
✌️ @sgasho, you can now perform try builds on this pull request! You can now post |
|
☔ The latest upstream changes (presumably #150541) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@bors try jobs=dist-aarch64-apple |
This comment has been minimized.
This comment has been minimized.
30f87bc to
31d3fde
Compare
|
@bors try jobs=dist-aarch64-apple |
This comment has been minimized.
This comment has been minimized.
Add dist step for enzyme, including aarch64-apple-darwin try-job: dist-aarch64-apple
This comment has been minimized.
This comment has been minimized.
bc7b483 to
98144d4
Compare
|
@bors try jobs=dist-aarch64-apple |
This comment has been minimized.
This comment has been minimized.
Add dist step for enzyme, including aarch64-apple-darwin try-job: dist-aarch64-apple
|
💔 Test for 2c9be7e failed: CI. Failed job:
|
|
@bors try jobs=dist-aarch64-apple |
This comment has been minimized.
This comment has been minimized.
Dist Enzyme for macOS with unversioned LLVM dylib try-job: dist-aarch64-apple
|
(please feel free to ping Ralf jung whenever you think you have an artifact that you think should work for them) |
|
@RalfJung https://github.com/rust-lang/rust/actions/runs/23104042131 |
|
Nice, it seems like Miri passed. Can you please run some (or all if not too slow) of the UI or pretty tests a few times under the old and new tool chain? I'd expect a diff to be more noise than anything based on what was discussed previously, but we should confirm that the diff isn't too big. Just post all times, but the minimal times are likely the most relevant ones. |
| @@ -72,7 +75,8 @@ pub fn walk_native_lib_search_dirs<R>( | |||
| || sess.target.os == Os::Linux | |||
There was a problem hiding this comment.
The comment above needs to be updated with an explanation for why we also do this for macOS+libLLVM.dylib.
ah, I made a mistake around how to use new toolchain. I only got pretty test results today.
I hope I can share the results of ui tests by tomorrow. |
|
mmm....Am I doing the right thing. What is the right command I should run? ./x test --stage 0 \
--force-rerun \
--set build.compiletest-allow-stage0=true \
--set "build.rustc=$(rustup which --toolchain (old or new toolchain name. new one is from CI tarballs from the latest commit) rustc) \
--set "build.cargo=$(rustup which --toolchain (old or new toolchain name) cargo)" \
--build-dir /tmp/somewhere \
tests/ui |
|
@jieyouxu did you add some logic (like fusing of multiple tests) to the UI test suite? I wouldn't expect it, but I'm also a bit surprised that UI times stayed the same, while pretty tests got so much slower. @sgasho can you maybe also test some small hello world project or something similar as a tie breaker? Edit: wait, the new tool chain is probably in a different folder, so I'd guess some MacOS Anti-Virus is checking it and killing performance. I vaguely remember these issues, there should be something in a dev guide about it. https://nnethercote.github.io/2025/09/04/faster-rust-builds-on-mac.html Still not sure why it's only showing up in pretty tests, but I guess that could explain the 3x perf difference between the official nightly and your manually downloaded one? |
Not that I know of, at least not for non-parallel ui tests |
…zyme build" This reverts commit b483e2f.
|
I reran tests/pretty and realized the time difference is likely dominated by failure output. I did not so much care about the failures because the number of failures were same...I thought it was just due to the time lag between this branch and the old toolchain(nightly). But that was the problem and I overlooked it. So, I updated the nightly and pull rebased main into this branch. I checked that all 102 tests have been passed in old toolchain(around 2.5sec). However, 4 tests failed in new one(around 6sec). I'm checking the failures. |
5ffae10 to
501934f
Compare
|
@bors try jobs=dist-aarch64-apple,aarch64-apple, |
This comment has been minimized.
This comment has been minimized.
Dist Enzyme for macOS with unversioned LLVM dylib try-job: dist-aarch64-apple try-job: aarch64-apple try-job:
./x test tests/pretty locally on this branch has been passed in 7sec |
|
well, I git cloned r-l/r into another directory and tested it with ./x test tests/pretty. It also took 7sec, which means there are no time diffs between these branches. |
| continue; | ||
| } | ||
|
|
||
| let name = |
There was a problem hiding this comment.
Add a comment explaining why we overwrite it here.
|
Yeah, I was expecting perf differences between local builds vs CI artifacts, only local vs local or CI vs CI makes sense, with the latter being the source of truth since that's what we distribute. Thanks for investigating and testing all combinations. Seems like we have no significant perf change in these stress tests, and real-world application would mask any latency increase (if it even exists) even better. A few local test failures are to be expected. If you download a CI artifact and test it on your local clone, then all those tests will fail that were modified between your last sync and the artifact creation time. As long as it's just a few and CI is happy then the PR should be good. Mads had a few weeks and multiple pings but didn't specify what he meant with "multiple reasons" for not doing this change here: #152768 (comment). However, IIUC he reverted his opinion in #153077 (comment) and only cared about the perf, for which we have new numbers here (aka no change). The miri / downstream failure seems to also be revolved, so I am not aware of any other blockers. The change however unblocks distributing autodiff without increasing rustc artifact size for all users (requested by the infra team), and it also unblocks further projects like distributing clang (or lld) without repeatedly including LLVM. @sgasho can you address Bjorn and my feedback, squash the commits and remove the enzye line for now? Then Ralf can run a final miri check if he wants and we can approve it in a few days if no one raises any new concerns. |
View all comments
Follow-up to #152768.