Repository navigation
ci(hipblaslt): use host ASAN with XNACK disabled - #13126
Merged
Merged
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #13126 +/- ##
=========================================
Coverage 72.12% 72.12%
=========================================
Files 2861 2862 +1
Lines 472089 472193 +104
Branches 69803 69826 +23
=========================================
+ Hits 340449 340544 +95
+ Misses 107910 107904 -6
- Partials 23730 23745 +15
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
talumbau
self-requested a review
October 6, 2026 21:11
talumbau
approved these changes
Oct 6, 2026
newling
reviewed
Oct 6, 2026
newling
left a comment
newling
left a comment
Contributor
There was a problem hiding this comment.
This PR disables ASAN for device code, which is fine by me if it gets CI green with host asan coverage again. I got an agent to review this (see here) and it didn't flag anything. i've been iterating on another solution with agents the last few days, and the PR that came out of that is a 1-liner that changes to runner pool: #12813. I don't understand why it helps, but it does seem to reduce the failure rate to 0 (only had a few runs with it).
newling
approved these changes
Oct 6, 2026
Contributor
|
Hi @sa-faizal, thanks for this! And thanks @newling for the review and the #12813 comparison. That's really useful. One data point to add. On Sep 30, after #12728 merged, we saw 6 straight hangs in this lane across 4 hosts in I'm good with this going in for the per-PR lane. Two asks:
I'd rather not move this lane to a different pool. If we decide we need gfx90a device ASAN back, I think we should ask DevOps for a pool that's explicitly set up for full ASAN with XNACK on (probably nightly), and track that separately rather than hold this PR. |
Contributor
|
Can we |
Contributor
TheRock Submodule Bump ActivityNewest first
|
tony-davis
added a commit
that referenced
this pull request
Oct 9, 2026
…findings Both Improvement Roadmap sections now say they are not yet a plan: testing for hipBLASLt and TensileLite is being redesigned under AIHPBLAS-5028, and the items listed are tactical fixes found while documenting the current state. Their subsections are grouped by kind of work instead of by time horizon. Also bring the docs in line with develop after the rebase: the hipBLASLt ASAN lane is host-only ASAN since #13126, so six passages that still described full ASAN on gfx90a per PR are updated, and the sanitizer table keeps every lane as non-gating, which matches the develop ruleset. Two TensileLite links still used the old Tensile/ path. Co-authored-by: Cursor <cursoragent@cursor.com>
1 task done
tony-davis
added a commit
that referenced
this pull request
Oct 21, 2026
…testing redesign (#12997) > **Reading this PR:** the diffs are hard to follow, so here are both files rendered as they stand on this branch. The links follow the branch, so they always show the latest commit. > > - [projects/hipblaslt/TESTING.md](https://github.com/ROCm/rocm-libraries/blob/users/tony-davis/AIHPBLAS-5028-testing-md-tracking/projects/hipblaslt/TESTING.md) > - [projects/hipblaslt/tensilelite/TESTING.md](https://github.com/ROCm/rocm-libraries/blob/users/tony-davis/AIHPBLAS-5028-testing-md-tracking/projects/hipblaslt/tensilelite/TESTING.md) JIRA ID : AIHPBLAS-5028 ## Motivation hipBLASLt is AMD's matrix-multiply (GEMM) library, and TensileLite is the Python code generator that writes its GPU kernels. Each has a TESTING.md that describes how it's tested today and ends with a "Known Risks and Gaps" table and an "Improvement Roadmap". While checking those against current CI, I found a lot of specific shortcomings, and most of them had no ticket. This PR records them and gives each one a ticket. It's the first step of a bigger effort, though, not the plan. Testing across these two products is pretty scattered, and I think it needs a top-down redesign rather than a longer list of fixes. That redesign comes in the PRs stacked on this one: a shared testing plan, an inventory of every suite, and a coverage model. So the roadmap sections here are framed as tactical findings. They aren't a commitment or a priority order. ## Technical Details Docs only. No code, test, or CI configuration changes. - **Every gap row and roadmap item names a ticket.** New work items are under the epic AIHPBLAS-5028, plus #12996 for the out-of-date required-check list in docs/gardening.md. Where an existing ticket already covered a gap, the row uses it. For example, the rows about Math CI's `preliminary` job point at AIHPBLAS-3354, which moves TensileLite testing to TheRock CI, since Math CI is being retired. - **The roadmap sections are reframed.** Each one now opens by saying it's not yet a plan, and its items are grouped by kind of work ("Process and policy fixes", "Structural changes", "Larger investments") instead of near, medium, and longer term. - **Six rows and three roadmap items are removed.** Four of the gaps are closed on develop: TheRock now runs hipBLASLt tiers through CTest, submodule-bump PRs run more tests rather than fewer, gfx950 runs per PR in Multi-Arch CI, and TheRock's installed-artifact lane now has the snapshot-test plugin (syrupy). `preliminary`'s target-branch condition no longer exists. The last one is a Math CI job name we don't plan to change, since Math CI is going away. - **Four rows are added** from existing tickets: large-size address arithmetic (AIHPBLAS-4988), tests of the public API itself (AIHPBLAS-3579), the library-logic check skipping derived-parameter assignment (AIHPBLAS-3575), and no sanitizer lane being a required check (AIHPBLAS-1079). - **Body text that no longer matched CI is corrected.** The ones most likely to change how someone reads a CI result: - The TensileLite coverage floor blocks a merge. Its lane rolls up to `Component CI Summary`, which the develop ruleset requires. The floor is 82%, not 75%. - The required checks are `Math CI Summary`, `pre-commit`, `Multi-Arch CI Summary` and `Component CI Summary`. Neither AddressSanitizer (ASAN) lane is one of them. - gfx950 runs per PR in TheRock Multi-Arch CI at the `standard` tier, as of the TheRock pin bump in #12826. - Math CI schedules a PR by its `project:` label, and the labeler doesn't give that label to `shared/rocroller` or `shared/mxdatagenerator`. So a PR that only touches those gets no hipBLASLt Math CI jobs at all. - Smaller fixes: Math CI runs multi-GPU tests on two gfx950 GPUs, the mutation-testing slice is ten files, one test file and one corpus check were removed in #11777, a dead cmake link, and the current rocPTS nightly details. - **Rebased onto develop.** develop changed both files after this PR opened: the TensileLite lowercase namespace move (#12150, #13399), and the ASAN lane change in #13126. I kept develop's paths and lane facts. One exception: #13126 marks the gfx90a HOST_ASAN lane as gating, but `hipBLASLt ASAN CI Summary` isn't a required check in the develop ruleset, so the table says it doesn't gate. I also updated six other passages that still described full ASAN on gfx90a per PR, and two TensileLite links that still used the old `Tensile/` path. ## Test Plan Docs only, so the checks are about accuracy and links: - `pre-commit run --files` on both files. - Resolve every relative link and in-document anchor in both files. - Resolve every Jira key and GitHub link through the Jira REST API and `gh api`. - Check each corrected statement against develop, TheRock at the pinned commit, rocJenkins at its live branch, and the develop ruleset (`gh api repos/ROCm/rocm-libraries/rules/branches/develop`). ## Test Result - `pre-commit` passes. The Markdown files fall outside every hook's file filter, so I also checked trailing whitespace, links, and anchors with a script. - All Jira keys and GitHub references resolved when this PR was first checked, and the rebase and reframing added none. - The original corrections were checked against develop at f4ec50c and TheRock c9bde5b. After the rebase I rechecked the ASAN lanes and the required checks against develop at 67b685e. I haven't re-audited every other statement against TheRock 76bd097, which develop pins now. - Automated GitHub checks are pending. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests. ## Risk level Low (1/5): documentation only. No product, test, CI, or device behavior changes. The main risk is that one of the corrections is itself wrong, which is what review is for. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
rkayaith
added a commit
that referenced
this pull request
Oct 28, 2026
Replace the generic BUILD_ADDRESS_SANITIZER condition with an explicit THEROCK_SANITIZER STREQUAL "ASAN" check, matching hipBLASLt. Full TheRock ASAN selects the xnack+ default and supported target lists; HOST_ASAN uses the regular lists. Preserve the standalone HIPSPARSELT_ENABLE_ASAN path. Current TheRock does not set BUILD_ADDRESS_SANITIZER for hipSPARSELt, so HOST_ASAN already selects regular targets with the old condition. This change does not enable host ASAN or change current host-ASAN CI targets. The condition selects default and allowed targets; it does not rewrite explicit GPU_TARGETS supplied by TheRock. This completes the hipSPARSELt target distinction requested in Step 1 of AIHPBLAS-5081. The workflow's HOST_ASAN/XNACK settings and hipBLASLt target handling were already merged in #13126.
rkayaith
added a commit
that referenced
this pull request
Oct 29, 2026
Replace the generic BUILD_ADDRESS_SANITIZER condition with an explicit THEROCK_SANITIZER STREQUAL "ASAN" check, matching hipBLASLt. Full TheRock ASAN selects the xnack+ default and supported target lists; HOST_ASAN uses the regular lists. Preserve the standalone HIPSPARSELT_ENABLE_ASAN path. Current TheRock does not set BUILD_ADDRESS_SANITIZER for hipSPARSELt, so HOST_ASAN already selects regular targets with the old condition. This change does not enable host ASAN or change current host-ASAN CI targets. The condition selects default and allowed targets; it does not rewrite explicit GPU_TARGETS supplied by TheRock. This completes the hipSPARSELt target distinction requested in Step 1 of AIHPBLAS-5081. The workflow's HOST_ASAN/XNACK settings and hipBLASLt target handling were already merged in #13126.
rkayaith
added a commit
that referenced
this pull request
Oct 29, 2026
Replace the generic BUILD_ADDRESS_SANITIZER condition with an explicit THEROCK_SANITIZER STREQUAL "ASAN" check, matching hipBLASLt. Full TheRock ASAN selects the xnack+ default and supported target lists; HOST_ASAN uses the regular lists. Preserve the standalone HIPSPARSELT_ENABLE_ASAN path. Current TheRock does not set BUILD_ADDRESS_SANITIZER for hipSPARSELt, so HOST_ASAN already selects regular targets with the old condition. This change does not enable host ASAN or change current host-ASAN CI targets. The condition selects default and allowed targets; it does not rewrite explicit GPU_TARGETS supplied by TheRock. This completes the hipSPARSELt target distinction requested in Step 1 of AIHPBLAS-5081. The workflow's HOST_ASAN/XNACK settings and hipBLASLt target handling were already merged in #13126.
rkayaith
added a commit
that referenced
this pull request
Oct 29, 2026
Replace the generic BUILD_ADDRESS_SANITIZER condition with an explicit THEROCK_SANITIZER STREQUAL "ASAN" check, matching hipBLASLt. Full TheRock ASAN selects the xnack+ default and supported target lists; HOST_ASAN uses the regular lists. Preserve the standalone HIPSPARSELT_ENABLE_ASAN path. Current TheRock does not set BUILD_ADDRESS_SANITIZER for hipSPARSELt, so HOST_ASAN already selects regular targets with the old condition. This change does not enable host ASAN or change current host-ASAN CI targets. The condition selects default and allowed targets; it does not rewrite explicit GPU_TARGETS supplied by TheRock. This completes the hipSPARSELt target distinction requested in Step 1 of AIHPBLAS-5081. The workflow's HOST_ASAN/XNACK settings and hipBLASLt target handling were already merged in #13126.
rkayaith
added a commit
that referenced
this pull request
Oct 29, 2026
Replace the generic BUILD_ADDRESS_SANITIZER condition with an explicit THEROCK_SANITIZER STREQUAL "ASAN" check, matching hipBLASLt. Full TheRock ASAN selects the xnack+ default and supported target lists; HOST_ASAN uses the regular lists. Preserve the standalone HIPSPARSELT_ENABLE_ASAN path. Current TheRock does not set BUILD_ADDRESS_SANITIZER for hipSPARSELt, so HOST_ASAN already selects regular targets with the old condition. This change does not enable host ASAN or change current host-ASAN CI targets. The condition selects default and allowed targets; it does not rewrite explicit GPU_TARGETS supplied by TheRock. This completes the hipSPARSELt target distinction requested in Step 1 of AIHPBLAS-5081. The workflow's HOST_ASAN/XNACK settings and hipBLASLt target handling were already merged in #13126.
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
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.
ISSUE ID: #12727
Motivation
The hipBLASLt per-PR ASAN lane has been hanging under full ASAN with
HSA_XNACK=1. This switches it to host-only ASAN with XNACK off, matching the coverage already described inTESTING.md.GPU correctness tests still run, and ASAN still checks CPU memory accesses. The trade-off is that no gfx90a device-ASAN runtime tests remain in CI. Before restoring that coverage, we need to review runner capacity and full-ASAN stability with DevOps, potentially for a dedicated nightly pool. The cause of the hangs remains under investigation.
#12728 added shorter timeouts and diagnostics. #12813 offers an alternative runner-pool workaround. The investigation is tracked in TheRock-Infra #978.
Technical Details
HOST_ASANartifacts andHSA_XNACK=0in the focused lane.xnack+targets for host-only ASAN; preserve existing full-ASAN behavior.TESTING.mdand remove the stale gfx942ci:asanrow.Test Plan
Run the existing gfx90a build and smoke tests, plus whitespace and applicable pre-commit checks.
Test Result
Build and all 950 smoke tests passed on
c92deaa. The documentation follow-up passes whitespace checks; pre-commit completed with no applicable hooks. CI for the latest commit must pass before merge.Submission Checklist
Risk level
Medium (3/5): retains host-ASAN and GPU correctness checks, but removes device-ASAN coverage from the gfx90a lane. A passing run does not prove the hangs are fixed.