Skip to content

fix(rocke): Remove silent GFX950 fallback in fusion lowering#9245

Open
ecamartins wants to merge 5 commits into
developfrom
users/ecamartins/remove-gfx950-fallback-in-fusion-lowering
Open

fix(rocke): Remove silent GFX950 fallback in fusion lowering#9245
ecamartins wants to merge 5 commits into
developfrom
users/ecamartins/remove-gfx950-fallback-in-fusion-lowering

Conversation

@ecamartins

@ecamartins ecamartins commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Motivation

In rocKE's fusion_lowering.py, the GPU architecture silently falls back to gfx950, which can mask configuration or environment issues. Hence, this change removes the silent fallback and throws a descriptive error message instead.

Note: This gfx950 fallback exists throughout rocKE. A follow-up ticket will work to remove the remaining fallbacks.

Technical Details

In fusion_lowering.py, there were several cases of the following forms:

 dev = get_device_arch() or "gfx950"
 build_gfx = dev if dev in known_arches() else "gfx950"

In the above case, if get_device_arch() returns None (e.g., due to no GPU) or if the given architecture is not in the set of known architectures, then there is a silent fallback to gfx950. This can mask environment or configuration issues.

Instead of using the silent fallback, we throw a descriptive error message through the addition of the function validate_arch. This will inform the user of how to properly configure their environments.

Test Plan

Addition of the following tests:

  • Unit tests in test_rocke_multiarch.py to test validate_arch
  • Test cases in test_rocke.py's TestLoweringRegistryBuild to validate cases where validate_arch throws.
    • Despite documentation stating that test_rocke.py does not require a GPU, I discovered that tests that call a lowerer's build method require a GPU (i.e., the build method creates a KernelLauncher object whose constructor calls hipModuleLoad). So, I refactored the TestLoweringRegistryBuild class to mock the calls to the hip functions to remove the GPU dependency.

Test Result

All tests in test_rocke.py and test_rocke_multiarch.py pass.

Submission Checklist

JIRA ID : AICK-1541

@therock-pr-bot

therock-pr-bot Bot commented Jul 9, 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 Jul 9, 2026

Copy link
Copy Markdown

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

In rocKE's fusion_lowering.py, the architecture silently falls back to
gfx950, which can mask configuration or environment issues. This change
removes the silent fallback and throws an error instead via the new
validate_arch function.

Unit tests for validate_arch were added. Additional test cases were
added in test_rocke.py's TestLoweringRegistryBuild to test cases where
validate_arch throws. Reliance on the GPU was removed from this test
class via mocks to functions that call the hip runtime api.

A follow-up ticket will address a full sweep to remove all cases of the
silent gfx950 fallback.
@ecamartins ecamartins force-pushed the users/ecamartins/remove-gfx950-fallback-in-fusion-lowering branch from d6e233c to 114b99c Compare July 9, 2026 21:07
@yraparti

yraparti commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Critical:

  • platform/AGENTS.md states under Hard rules: "Default arch is gfx950 … for on-GPU runs, prefer the local device via get_device_arch() and fall back to gfx950." This PR removes that fallback. The change is intentional and a follow-up ticket is noted, but the guidance doc now contradicts the code. Consider updating AGENTS.md in this PR (or the follow-up) so the two don't drift.

Nits:

  • validate_arch(arch: str) — the signature says str but None is a first-class, handled input. Type it Optional[str].
  • Docstring wording in _build_region_hsaco: says the LLVM lowering "throws an error when … otherwise it throws an error." The error is actually raised by validate_arch before lowering, not by the lowering path. Also, Python convention is "raises" rather than "throws." Small accuracy fix.
  • Error message interpolates the raw tuple {known_arches()}; ', '.join(known_arches()) reads a bit cleaner for a user-facing message. Optional.
  • elif arch not in known_arches(): follows a raise, so it can just be if — trivial.

The following changes were made:
- Update validate_arch's parameter type hint to Optional[str]
since None is a valid argument.
- Update documentation wording for more clarity
- Update AGENTS.md file to indicate that fusion_lowering.py no
longer falls back to gfx950 for better agentic workflows
@ecamartins

Copy link
Copy Markdown
Contributor Author

@yraparti

Critical:

  • platform/AGENTS.md states under Hard rules: "Default arch is gfx950 … for on-GPU runs, prefer the local device via get_device_arch() and fall back to gfx950." This PR removes that fallback. The change is intentional and a follow-up ticket is noted, but the guidance doc now contradicts the code. Consider updating AGENTS.md in this PR (or the follow-up) so the two don't drift.

I've updated AGENTS.md to indicate that fusion_lowering.py does not fall back to gfx950.

Nits:

  • validate_arch(arch: str) — the signature says str but None is a first-class, handled input. Type it Optional[str].
  • Docstring wording in _build_region_hsaco: says the LLVM lowering "throws an error when … otherwise it throws an error." The error is actually raised by validate_arch before lowering, not by the lowering path. Also, Python convention is "raises" rather than "throws." Small accuracy fix.
  • Error message interpolates the raw tuple {known_arches()}; ', '.join(known_arches()) reads a bit cleaner for a user-facing message. Optional.
  • elif arch not in known_arches(): follows a raise, so it can just be if — trivial.

All required nits have been addressed.

@ecamartins ecamartins requested a review from yraparti July 10, 2026 17:01
Since calls to hipModuleLoad are now mocked, tests in
test_rocke.py no longer require a GPU. So, the _requires_gpu
guard can be removed.
@ecamartins ecamartins force-pushed the users/ecamartins/remove-gfx950-fallback-in-fusion-lowering branch from dfd4fce to d6d03ea Compare July 10, 2026 18:23
Comment on lines +189 to +192
*Note*: We are transitioning away from the `gfx950` fallback. Currently,
`rocke/platform/python/rocke/helpers/fusion_lowering.py` does not fall back to
`gfx950`; an exception is raised if a known device can't be detected. Future
work will remove remaining `gfx950` fallbacks.

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.

maybe we can remove this and just add a line saying pass target_architecture for non-gfx950 targets and beware of the silent fallbacks on non-gpus?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Done.

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.

2 participants