Skip to content

Pass ENVIRONMENT_MODIFICATION to hip-kernel-provider test registration - #12804

Merged
CMiservaAMD merged 1 commit into
developfrom
users/cmiserva/almiopen-2821/fix-hip-kernel-provider-windows-asan-test-env
Oct 7, 2026
Merged

CMiservaAMD merged 1 commit into
developfrom
users/cmiserva/almiopen-2821/fix-hip-kernel-provider-windows-asan-test-env

Conversation

@CMiservaAMD

Copy link
Copy Markdown
Contributor

Summary

hip-kernel-provider registered its CTest suites without passing ENVIRONMENT_MODIFICATION, unlike the three peer providers under dnn-providers/. In a Windows AddressSanitizer build this left the test executables unable to resolve the ASAN runtime DLL, and four suites exited 0xc0000135 (STATUS_DLL_NOT_FOUND) in under 0.05 s without reaching main. This passes the keyword at the hip-kernel-provider registration sites so they match the peers.

JIRA ID : ALMIOPEN-2821

Risk Assessment

Risk 2. TEST_ENVIRONMENT_MODIFICATION is set only when BUILD_ADDRESS_SANITIZER is enabled on Windows, and the registration helpers emit nothing when the variable is empty, so the change is a no-op in every non-ASAN and non-Windows configuration. No kernel selection, dispatch, default, or support-surface behaviour changes.

ASIC Coverage

Architecture-independent: the change alters the test process environment only. Standard PR CI is the appropriate bar and no multi-arch sweep is warranted on the merits.

Caveat a reviewer should know: no CI lane covers the affected configuration. The ASAN multi-arch workflows accept only linux_amdgpu_families, and therock-ci-windows.yml does not enable ASAN, so PR CI can confirm this is a no-op elsewhere but cannot verify the fix itself. Verification is local, on Windows / gfx1103.

Also note that four of the seven changed call sites (the gfx950, asm-sdpa and kernel-ingestor targets) are gated off in the verifying configuration and so were not exercised. They carry the identical omission and the identical fix.

Testing Summary

  • Existing shared coverage: shared/ctest/tests/test_parse_test_categories.py::test_cli_environment_modification_emitted already proves apply_test_category_labels propagates ENVIRONMENT_MODIFICATION into the generated CTestTestfile. No new test is added: the helper was never at fault, the defect was a caller omitting the keyword. Note this test covers the mechanism, not a re-omission at a call site.
  • Local Windows gfx1103 ASAN configure and build: 0 errors, 0 warnings.
  • Generated-output inspection: hip-kernel-provider's regenerated CTestTestfile.cmake carries 15 ENVIRONMENT_MODIFICATION entries where it previously carried none, with the same three path_list_prepend entries as the working peer miopen_plugin_tests_standard_suite.
  • Full ctest -L standard run on Windows gfx1103 ASAN, to confirm the affected suites now start.

Testing Checklist

  • Shared ctest helper coverage - python -m unittest test_parse_test_categories -k environment_modification - Status: Passed
  • Windows gfx1103 ASAN configure + build - cmake -E env CMAKE_BUILD_PARALLEL_LEVEL=4 cmake --build build/release-gfx1103-asan-newstd -j 4 - Status: Passed
  • Generated CTestTestfile inspection - manual diff against peer provider - Status: Passed
  • Windows gfx1103 ASAN standard suites - ctest -L standard - Status: In progress
  • PR CI - GitHub PR checks - Status: Pending

Technical Changes

  • dnn-providers/hip-kernel-provider/src/CMakeLists.txt: pass ENVIRONMENT_MODIFICATION at five registration sites (two apply_test_category_labels calls, three external-integration targets), and update an adjacent comment to name the keyword.
  • dnn-providers/hip-kernel-provider/src/integration_tests/CMakeLists.txt: the same at two gfx950 / asm-sdpa targets.

🤖 Generated with Claude Code

@therock-pr-bot

therock-pr-bot Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass PR does not contain code files — Unit Test auto-passed
🔎 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.

🙋 Wish to Override Policy?

@therock-pr-bot

therock-pr-bot Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

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

@CMiservaAMD
CMiservaAMD marked this pull request as ready for review October 1, 2026 13:59
@CMiservaAMD
CMiservaAMD requested review from a team as code owners October 1, 2026 13:59
@CMiservaAMD
CMiservaAMD enabled auto-merge (squash) October 1, 2026 22:57

@adickin-amd adickin-amd 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

…stration

hip-kernel-provider was the only provider under dnn-providers/ whose test
registration omitted ENVIRONMENT_MODIFICATION. Its peers (miopen-provider,
integration-tests) forward TEST_ENVIRONMENT_MODIFICATION alongside
TEST_ENVIRONMENT, which on Windows ASAN builds is what prepends the clang
runtime directory holding clang_rt.asan_dynamic-x86_64.dll -- plus the build
bin and ROCm bin directories -- onto the test's PATH.

Without it the generated suites started with an unmodified PATH and could not
resolve the ASAN runtime DLL. hip_kernel_provider_tests_standard_suite,
hip_kernel_provider_integration_tests_standard_suite,
hip-kernel-provider-external-integration_standard_suite and
hip-kernel-provider-asm-sdpa-external-integration_standard_suite all exited
with 0xc0000135 (STATUS_DLL_NOT_FOUND) in under 0.05 s, never reaching main().

TEST_ENVIRONMENT_MODIFICATION is set by dnn-providers/cmake/Sanitizers.cmake,
included at the provider root before add_subdirectory(src), so it is in scope
at every touched call site. It is empty on non-ASAN and non-Windows builds,
where each helper skips the property entirely.

The gfx950 attention-dense and gpu_ref external-integration targets carry the
same omission and are fixed alongside; they are not registered by every
configuration but fail identically where they are.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@CMiservaAMD
CMiservaAMD force-pushed the users/cmiserva/almiopen-2821/fix-hip-kernel-provider-windows-asan-test-env branch from a13df63 to 3e16f3e Compare October 6, 2026 17:31
@CMiservaAMD
CMiservaAMD merged commit 9cf012a into develop Oct 7, 2026
96 checks passed
@CMiservaAMD
CMiservaAMD deleted the users/cmiserva/almiopen-2821/fix-hip-kernel-provider-windows-asan-test-env branch October 7, 2026 10:52
adickin-amd added a commit that referenced this pull request Oct 7, 2026
Conflicts:
- HIP_MLOPS_ENGINE_test_categories_integration.yaml: keep the cascaded
  comprehensive tier (no full_*), which makes develop's full_Layernorm*
  carve-out (#13180) unnecessary.
- hip-kernel-provider/src/integration_tests/CMakeLists.txt: keep the
  all-tier GTEST_FILTER and take develop's ENVIRONMENT_MODIFICATION (#12804).
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