ROCm / HIP

HIP: C++ Heterogeneous-Compute Interface for Portability
https://rocmdocs.amd.com/projects/HIP/
MIT License
3.76k stars 539 forks source link

-x hip Should have same C++ std as -x c++ #2278

Closed ax3l closed 2 years ago

ax3l commented 3 years ago

Does this repo change the default of -std=c++<...> for clang++? I am trying to transition to the clang++ + hip::device target logic (#2275) with ECP AMReX & ECP WarpX.

The compiler identifies as Clang 12 but it builds by default in C++11.

My targets set

target_compile_features(${tgt} PUBLIC cxx_std_14)

and only with this clang++ in hip 4.1.0 I see no flags getting added to the command line.

Note that Clang 6+ already default to C++14: https://gist.github.com/ax3l/53db9fa8a4f4c21ecc5c4100c0d93c94

But -xhip does not it seems:

rocm-4.1.0/llvm/bin/clang++ -dM -E -xhip  /dev/null | grep -F __cplusplus
#define __cplusplus 201103L
#define __cplusplus 201103L

Vs.:

rocm-4.1.0/llvm/bin/clang++ -dM -E -x c++  /dev/null | grep -F __cplusplus
#define __cplusplus 201402L

My Workaround

export CXXFLAGS="-std=c++14"

before all CMake builds, because the target compile feature cxx_std_14 is currently ignored for -xhip objects.

Alternative work-around: CXX_STANDARD always adds -std=c++XX, even if the compiler default fulfills it

set_property(TARGET mytarget PROPERTY CXX_STANDARD 14)

The reason for that is that cxx_std_14 asks for feature support (at least a minimal version) while CXX_STANDARD is an exact request for a version.

pfultz2 commented 3 years ago

Does this repo change the default of -std=c++<...> for clang++?

This repo doesn't change the C++ version. That is set by the llvm compiler. Using -x hip sets the version to C++11 in llvm because that is the minimum version needed to use hip header files, but this can be overwritten with passing a different version like -std+c++14.

At the time, I believe hip was created, llvm used C++98 by default, which is why we bumped it to c++11. When llvm switched to C++14 by default, it seems they didn't update it for all languages.

and only with this clang++ in hip 4.1.0 I see no flags getting added to the command line.

That sounds like a bug in CMake. The cxx_std_14 should add at least the -std=c++14 flags(or later).

ax3l commented 3 years ago

Hi @pfultz2,

thanks for the background.

At the time, I believe hip was created, llvm used C++98 by default, which is why we bumped it to c++11. When llvm switched to C++14 by default, it seems they didn't update it for all languages.

Makes sense, I think we should now update this to C++14 just to stay in lockstep with the C++ frontend & avoid confusion.

That sounds like a bug in CMake. The cxx_std_14 should add at least the -std=c++14 flags(or later).

Depends, CMake does the following logic for all languages it supports: If the default compiler flags are fulfilling the requested compile feature, it will not add (verbose) extra flags. Since your CMake logic exposes itself as CXX targets (HIP is not yet a <LANG> in CMake), it will query clang++ (-x c++) and see it supports C++14. CMake does not know that your configs inject an -x hip later on in a specific target's interface compile flags and that this is thus a different language.

One can of course build work-arounds in CMake, but the least confusing for all downstream would just be to keep the -std=... defaults for -x hip and -x c++ the same, imho.

pfultz2 commented 3 years ago

Maybe @yxsamliu can provide feedback about upgrading llvm.

yxsamliu commented 3 years ago

I think it is reasonable to take c++14 as default language standard for HIP. I will try making that change.

emankov commented 3 years ago

I think it is reasonable to take c++14 as default language standard for HIP. I will try making that change.

I've already done it for hipify-clang. So, the minimum GNU C/C++ supported version now is 6.1 (was 5.4).

ax3l commented 3 years ago

Thanks! Just to clarify: With this issue, I am not suggesting to drop C++11 support. (That said, our projects are C++14 and newer anyway.)

I just would like to keep the -std=c++XX default for the hip and c++ compiler frontend in sync.

yxsamliu commented 3 years ago

understood

ax3l commented 3 years ago

Xref: Upstream PR in https://reviews.llvm.org/D103221

leofang commented 3 years ago

cc: @kmaehashi @takagi

ax3l commented 2 years ago

This is now fixed, thanks a lot!