Fix per-mol line search race - #355
Merged
scal444 merged 1 commit intoOct 2, 2026
Merged
Conversation
lineSearchPostEnergy computes the line-search verdict on thread 0 only and returns false on every other thread, but every thread of the block assigned its return value to the shared lineSearchConverged flag. The 127 false stores raced the single true store, so a converged line search was often recorded as not converged. The loop then re-evaluated the energy at the same trial step (lambda is unchanged on the converged path), reached the same verdict and raced again, until a true store happened to survive or MAX_LINESEARCH_ITERS was reached. The accepted step is the same either way, but each lost verdict costs a full energy evaluation. Only thread 0 now writes the flag; the existing barrier publishes it.
Contributor
 |
Collaborator
Author
|
This actually has a serious perf boost in-kernel for some cases. |
Collaborator
Author
|
I've got separate WIP for the flaky tests |
evasnow1992
approved these changes
Oct 2, 2026
evasnow1992
left a comment
evasnow1992
left a comment
Collaborator
There was a problem hiding this comment.
Thank you for the fix! Verified locally. Glad it was only impact performance not correctness.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This was not a correctness issue (luckily) but a performance one. Each thread in the block was writing to the same location, so if there was an exit condition from thread 0, sometimes it would be written, sometimes it wouldn't be, and the loop would continue unnecessarily.
I thought this would be drastic (1/128 chance of correct write), but apparently when all threads write the same location, the compiler aliases it to thread 0 per warp, so correct write was more like 1/4 chance.