Skip to content

Hip wrapper#20

Draft
scheibelp wants to merge 4 commits into
spack:mainfrom
scheibelp:hip-wrapper
Draft

Hip wrapper#20
scheibelp wants to merge 4 commits into
spack:mainfrom
scheibelp:hip-wrapper

Conversation

@scheibelp

@scheibelp scheibelp commented Jul 9, 2026

Copy link
Copy Markdown
Member

Used by spack/spack-packages#4411

Pipelines there fully passed with latest commit (d6cc6b2) 2b8ef4e (additional commit after that is for cleanup)

@scheibelp
scheibelp marked this pull request as draft July 9, 2026 23:57
@scheibelp

Copy link
Copy Markdown
Member Author

Question: should cxxflags attribute from compiler definitions (in packages.yaml) be added when detected language is HIP? or should there be a separate hipflags entry that adds flags to hip invocations. See https://github.com/spack/compiler-wrapper/blob/main/cc.sh#L789

Also, note that if it were processed like cxxflags: it would not work to add -x hip (because that gets placed after the source file, which is apparently important to amdclang++).

@haampie

haampie commented Jul 21, 2026

Copy link
Copy Markdown
Member

Did you check what CMake does? They implemented it twice, so presumably the second time they learned from mistakes from the past. See https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS.html#variable:CMAKE_%3CLANG%3E_FLAGS and various other pages, or sources.

Edit: I did that:

  • CMake passes -x hip, it's not extension based; we should do the same, otherwise the compiler wrapper may at some point need compiler+version specific parsing in case the real compiler changes behavior.
  • CMAKE_HIP_FLAGS_INIT is dervied from HIPFLAGS
  • CXXFLAGS is unrelated to HIPFLAGS.

That makes sense cause it's a different language.

This is the case for ~5 years, so I would say let's do -x hip only and fix upstream packages if needed, or deprecate them if too difficult.

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