Skip to content

Default HIP multi-arch workaround on Windows clang-cl#4941

Draft
DanyiLin wants to merge 3 commits into
developfrom
fix/windows-hip-multi-arch-workaround-default
Draft

Default HIP multi-arch workaround on Windows clang-cl#4941
DanyiLin wants to merge 3 commits into
developfrom
fix/windows-hip-multi-arch-workaround-default

Conversation

@DanyiLin

@DanyiLin DanyiLin commented Jun 4, 2026

Copy link
Copy Markdown

Motivation

WinRelease CI failed verify_offload_archs on migraphx_device.dll when MIGRAPHX_WORKAROUND_HIP_MULTI_ARCH_BUG was left at its previous default (OFF). The clang-cl HIP multi-arch /Fo workaround from #4920 must be enabled for typical Windows HIP SDK (clang-cl MSVC frontend) builds; requiring an explicit CMake flag is easy to miss in presets and CI.

Technical Details

  • Replace option(MIGRAPHX_WORKAROUND_HIP_MULTI_ARCH_BUG ... OFF) with cmake_dependent_option so the workaround defaults ON when building on Windows with CMAKE_CXX_COMPILER_ID Clang and CMAKE_CXX_COMPILER_FRONTEND_VARIANT MSVC (clang-cl), and OFF otherwise.
  • Existing compile-object override logic is unchanged; only the default for the option changes on the affected platform.
  • Related: Workaround clang-cl HIP multi-arch /Fo bug on Windows #4920 (introduced the workaround), UAI Jenkins WinRelease failure (job Add Tile #373).

Changelog Category

Add a CHANGELOG.md entry for any option other than Not Applicable

    • Added: New functionality.
    • Changed: Changes to existing functionality.
    • Removed: Functionality or support that has been removed. (Compared to a previous release)
    • Optimized: Component performance that has been optimized or improved.
    • Resolved Issues: Known issues from a previous version that have been resolved.
    • Not Applicable: This PR is not to be included in the changelog.

Test plan

  • WinRelease / Windows clang-cl configure shows MIGRAPHX_WORKAROUND_HIP_MULTI_ARCH_BUG = ON
  • verify_offload_archs passes for migraphx_device.dll on Windows CI
  • Linux / non-clang-cl builds still default the option to OFF

WinRelease CI failed verify_offload_archs on migraphx_device.dll when the workaround was OFF (UAI Jenkins #373). Enable cmake_dependent_option ON for WIN32 clang-cl MSVC frontend builds.

Co-authored-by: Cursor <cursoragent@cursor.com>
@DanyiLin DanyiLin marked this pull request as ready for review June 4, 2026 14:37
@DanyiLin DanyiLin requested a review from causten as a code owner June 4, 2026 14:37
@pfultz2

pfultz2 commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

This should not be enabled by default(as the workaround will still be enabled on the newer fixed clang-cl). If you are using an older clang-cl that is broken then you need to explictly add -DMIGRAPHX_WORKAROUND_HIP_MULTI_ARCH_BUG=On to your cmake command when building migraphx.

@DanyiLin DanyiLin marked this pull request as draft June 4, 2026 16:36
@DanyiLin DanyiLin marked this pull request as ready for review June 4, 2026 17:27
Lin, Danyi and others added 2 commits June 4, 2026 14:40
The cmake_dependent_option condition used unquoted MSVC, which expanded to
the MSVC variable instead of the frontend variant string and left the
workaround OFF in CI.

Co-authored-by: Cursor <cursoragent@cursor.com>
@DanyiLin DanyiLin marked this pull request as draft June 4, 2026 19:41
@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4941   +/-   ##
========================================
  Coverage    92.61%   92.61%           
========================================
  Files          587      587           
  Lines        30410    30410           
========================================
  Hits         28164    28164           
  Misses        2246     2246           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants