Move one simplification call in lowering. - #9355
Conversation
There was a problem hiding this comment.
This should be moved up above the now-final simplification
There was a problem hiding this comment.
Andrew argued that the string should change, instead of moving this. @abadams
There was a problem hiding this comment.
Suggest moving it after the custom passes and changing string to "Conceptual stmt:", because it's what you get if you compile to conceptual_stmt.
There was a problem hiding this comment.
Named it:
debug(1) << "Lowering after reaching conceptual Stmt:\n"
<< s << "\n\n";As Claude is grepping for "Lowering after" to find the different sections of the lowering phase.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9355 +/- ##
==========================================
+ Coverage 70.08% 70.10% +0.01%
==========================================
Files 259 260 +1
Lines 79158 79295 +137
Branches 19293 19328 +35
==========================================
+ Hits 55477 55587 +110
- Misses 17886 17927 +41
+ Partials 5795 5781 -14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A worker that found a job it could run but for an unavailable semaphore went to sleep on one of the two idle worker condition variables (the A/B teams), chosen by pool-size bookkeeping unrelated to why it slept. A semaphore release only ever broadcast wake_a_team, so a semaphore-blocked worker demoted to the B team was never woken by the release that made its job runnable, and the pipeline could deadlock with every thread parked. The A/B teams model idle capacity (no runnable work), which is a different state from being blocked on an external event. Give blocked-on-semaphore workers their own wait channel and wake it on every release, alongside stalled owners. Genuinely-idle A/B-team workers are never waiting on a semaphore, so they are left undisturbed. This also removes the old 0 -> 1-transition gate on the release wakeup, which was independently unsound for acquires of count > 1 satisfied by several count-1 releases in a row. Adds test/correctness/async_deadlock.cpp, pinned to a low thread count (where the pool reshuffles teams and reliably exposes the bug). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Deadlocks exposed here fixed by #9372 |
LLVM 21's LoopAccessAnalysis::getStrideFromAddRec() queries a scalable-vector access type's fixed element count without guarding against ScalableVectorType, hitting a fatal assertion (TypeSize::getFixedValue on a scalable TypeSize) inside LoopLoadEliminationPass, which the default O3 pipeline always runs regardless of Halide's own loop-opt settings. Fixed upstream in LLVM 22. Loop load elimination looks for opportunities to forward a store to a later load across loop iterations, which Halide-generated code essentially never benefits from, so skip it entirely on affected SVE targets under the buggy LLVM versions via PassInstrumentationCallbacks::registerShouldRunOptionalPassCallback rather than crash (or, in a no-asserts LLVM build, silently compute a bogus stride). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Andrew Adams <andrew.b.adams@gmail.com>
… conceptual stmt'
34b2370 to
84cc3b3
Compare
…o mcourteaux/move-simplify
|
Waiting for #9372 to land to update this branch, but this seems to work now. @abadams please take note of Claude's initiative to disable a lower pass in LLVM 21, due to a bug fixed in 22 regarding SVE on ARM. I'll open a PR separately for this LLVM pass skip and remove the skips in all the tests to see if that makes the test suite pass. |
3 participants
When debugging some stuff not part of this PR, I spotted a missing simplification, which Andrew determined to be AFTER hoist_loop_invariant_xxx. This moves the simplification in the right spot.
Breaking changes
I hope none, let's see.
These do not necessarily disqualify a PR from being merged, but they should at
least be tagged with the
release_noteslabel.Checklist