Skip to content

fix(stinkytofu): Enable ST waitcnt insertion by default#8804

Open
pschang-phy wants to merge 4 commits into
developfrom
users/pochang/enable_st_waitcnt
Open

fix(stinkytofu): Enable ST waitcnt insertion by default#8804
pschang-phy wants to merge 4 commits into
developfrom
users/pochang/enable_st_waitcnt

Conversation

@pschang-phy

@pschang-phy pschang-phy commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

JIRA ID : AIHPBLAS-3664

Motivation

Enable StinkyTofu waitcnt insertion, currently only on region loopWithPrefetch+noLoadLoopBody, by default for TensileLite.

Technical Details

Based-on #9407 to track returning atomics in waitcnt dataflow.

  • Set DisableSTWaitCnt default to False so ST waitcnt insertion runs by default.
  • Updated CFG block splitting to reserve existing block/label names and generate unique .fallthrough labels for synthetic fallthrough blocks.
  • Updated tensor wait dataflow so blocks with untagged tensor anchors keep live tensor queues instead of restoring the frozen sweep-0 tensor state, allowing loop-carried tensor loads to trigger the conservative s_wait_tensorcnt 0 fallback.

Test Plan

Test Result

Submission Checklist

@codecov-commenter

codecov-commenter commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

❌ Your project check has failed because the head coverage (76.92%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #8804   +/-   ##
========================================
  Coverage    69.59%   69.59%           
========================================
  Files         2771     2771           
  Lines       452699   452699           
  Branches     66658    66658           
========================================
  Hits        315022   315022           
  Misses      117337   117337           
  Partials     20340    20340           
Flag Coverage Δ *Carryforward flag
TensileLite 34.32% <ø> (-<0.01%) ⬇️ Carriedforward from 03e2423
TensileLite-CPP 38.06% <ø> (ø)
TensileLite-Unit 64.67% <100.00%> (ø)
hipBLAS 90.62% <ø> (ø) Carriedforward from 03e2423
hipBLASLt 34.63% <ø> (ø)
hipCUB 82.68% <ø> (ø) Carriedforward from 03e2423
hipDNN 86.35% <ø> (ø) Carriedforward from 03e2423
hipFFT 47.34% <ø> (ø) Carriedforward from 03e2423
hipRAND 76.12% <ø> (ø) Carriedforward from 03e2423
hipSOLVER 69.18% <ø> (ø) Carriedforward from 03e2423
hipSPARSE 86.27% <ø> (ø) Carriedforward from 03e2423
rocBLAS 47.95% <ø> (ø) Carriedforward from 03e2423
rocFFT 47.61% <ø> (ø) Carriedforward from 03e2423
rocRAND 57.01% <ø> (ø) Carriedforward from 03e2423
rocSOLVER 76.92% <ø> (ø) Carriedforward from 03e2423
rocSPARSE 72.59% <ø> (ø) Carriedforward from 03e2423
rocThrust 91.36% <ø> (ø) Carriedforward from 03e2423

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines Coverage Δ
...slt/tensilelite/Tensile/Common/GlobalParameters.py 98.43% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pschang-phy
pschang-phy marked this pull request as ready for review June 25, 2026 15:15
@pschang-phy
pschang-phy requested a review from a team as a code owner June 25, 2026 15:15
@pschang-phy
pschang-phy requested a review from cycheng June 25, 2026 15:15
@pschang-phy
pschang-phy force-pushed the users/pochang/enable_st_waitcnt branch 2 times, most recently from 17bb22c to cb2f93a Compare June 29, 2026 01:50
@therock-pr-bot

therock-pr-bot Bot commented Jun 29, 2026

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
🌿 Branch Name ✅ Pass
📝 PR Title/Description ✅ Pass
Forbidden Files ✅ Pass
🧪 Unit Test ✅ Pass
🔎 pre-commit ✅ Pass
🚫 Draft PR 🔜 To Be Enabled
🚩 Feature Flag 🔜 To Be Enabled
📊 Code Coverage 🔜 To Be Enabled
🤖 therock-pr-bot ✅ Pass

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

@therock-pr-bot

therock-pr-bot Bot commented Jun 29, 2026

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@pschang-phy
pschang-phy force-pushed the users/pochang/enable_st_waitcnt branch from cb2f93a to e356e2d Compare June 30, 2026 01:36
@pschang-phy
pschang-phy force-pushed the users/pochang/enable_st_waitcnt branch 2 times, most recently from e592081 to 0488618 Compare July 13, 2026 08:28
@pschang-phy
pschang-phy requested a review from a team as a code owner July 13, 2026 08:28
@pschang-phy
pschang-phy force-pushed the users/pochang/enable_st_waitcnt branch 5 times, most recently from 99b8429 to 2e1029a Compare July 14, 2026 09:01
Comment thread shared/stinkytofu/docs/user/stinky-waitcnt-insertion-pass.md Outdated
Comment thread shared/stinkytofu/docs/user/stinky-waitcnt-insertion-pass.md Outdated
Comment thread shared/stinkytofu/src/transforms/asm/CFGBuilderPass.cpp Outdated
Comment thread shared/stinkytofu/src/transforms/asm/CFGBuilderPass.cpp Outdated
Comment thread shared/stinkytofu/src/transforms/asm/CFGBuilderPass.cpp Outdated
@cycheng

cycheng commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

@jaopaulolc, @ThanHenderson, could you help us review this PR, in particular, the "RequiresXCntForVolatileVMEM" behavior?

In this PR, we enable the WaitCnt insertion pass by default for all SIA optimization levels in stinkytofu. This means we first remove all existing wait instructions, including the s_wait_xcnt 0, and then run the WaitCnt insertion pass to regenerate the required waits.

However, we're not completely sure about StreamK's policy. Does StreamK only insert s_wait_xcnt before every atomic store and RMW instruction, or are there other special cases where it also inserts s_wait_xcnt?

Thank you!

using namespace stinkytofu;
using namespace stinkytofu::waitcnt;

bool isVolatileVmemAtomic(const StinkyInstruction& inst) {

@jaopaulolc jaopaulolc Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addRequiredXcntDrains only re-emits s_wait_xcnt 0 before ops matched by isVolatileVmemAtomic (MUBUF/FLAT/GLOBAL atomics). That is narrower than StreamK's actual policy ("drain before volatile VMEM", not just atomics), so correctness today rests on an implicit, unenforced region invariant rather than on this predicate. I verified the mechanics in generated gfx1250 assembly (dynamic-StreamK SGEMM, built stinkytofu-opt on both develop and this PR head).

  • Empirically, the one in-region volatile-VMEM site is the dynamic work-queue atomic global_atomic_inc_u32. Pre-PR develop with the ST pass ON strips its hand-written s_wait_xcnt and never regenerates it (a real latent dropped-drain bug); this PR restores it tightly immediately before the atomic. So the PR also fixes a pre-existing dropped-drain, and that site is covered precisely because it is an atomic.

  • The 2 non-atomic volatile flag ops — buffer_store_b32 (partial-tile release) and buffer_load_b32 (acquire/poll) — retain their s_wait_xcnt in every build I generated (pass on/off, develop/PR) only because they sit in the epilogue, OUTSIDE the {loopWithPrefetch, noLoadLoopBody} region this pass processes. They are IF_MUBUFStore/IF_MUBUFLoad, so isVolatileVmemAtomic excludes them; region scoping, not the predicate, is the guardrail.

  • The gap is silent and I reproduced it: a crafted in-region non-atomic flag store has its hand-written drain stripped by StinkyRemoveWaitCntPass and NOT regenerated. If any volatile-VMEM flag op ever moves into the covered region (in-loop variant, or code pulled into noLoadLoopBody), XNACK-replay reordering around the partial-tile release/acquire handshake -> cross-workgroup data race. The added FileCheck (buffer_store_b32 -> CHECK-NOT: s_wait_xcnt) codifies this hole.

  • Recommendation: broaden the predicate from "volatile VMEM atomics" to true "volatile VMEM" (atomics plus glc/dlc/dev-scope buffer/flat/global load/store), decoupling correctness from the region invariant. Beyond that, the current tests only cover the in-region atomic path; add positive-regression coverage for the out-of-region non-atomic sites so a refactor that pulls them in-region or drops their drain is caught:

    • Add an end-to-end TensileLite StreamK gfx1250 codegen regression (golden .s / FileCheck via the Tensile path) asserting s_wait_xcnt is present before ALL 5 sites — the 3 RMW atomics AND the 2 non-atomic flag ops (buffer_store_b32 release, buffer_load_b32 acquire) — so the invariant that these flag ops stay out-of-region and keep their drains is actually guarded, not assumed.
    • Add a guard/assert encoding the invariant directly: fail if a volatile-VMEM op requiring an xcnt drain appears inside a processed region without a regenerated drain (an in-region assert/XFAIL for a volatile non-atomic store/load).
    • Reconcile the PR's own unit FileCheck (buffer_store_b32 -> CHECK-NOT: s_wait_xcnt): it currently asserts the opposite of what StreamK needs should that op ever be in-region. Scope it explicitly to the out-of-region/no-drain-required case (or flip it once the predicate is broadened) so the two tests don't encode contradictory invariants.

@cycheng cycheng Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaopaulolc , Thank you! After some discussion, we decided to implement a dedicated hazard resolution pass in the next PR. As a temporary workaround, we won't modify streamk or tensilelite's xcnt placement in the non-SIA4 code generation path.

@jaopaulolc jaopaulolc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise.

pschang-phy and others added 4 commits July 25, 2026 23:22
Keep live tensor queues for blocks with untagged tensor anchors
instead of restoring the frozen sweep-0 tensor state, so loop-carried
tensor loads can still force the conservative s_wait_tensorcnt 0 fallback.
Revert the temporary waitcnt-insertion xcnt handling and keep
hand-authored s_wait_xcnt drains on non-SIA4 paths.

SIA4 still removes all waits.
SIA0 preserves tensilelite and streamk's emitted xcnt drains.

TODO: land a dedicated hazard pass handles them and remove this
      workaround.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants