Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move from hipcc to amdclang #1920

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

Conversation

ellosel
Copy link
Contributor

@ellosel ellosel commented May 17, 2024

The goal of this change set is to move the default compiler from hipcc to amdclang++. The most significant difference between hipcc and amdclang are the flags used when invoking the compiler wrapped by hipcc. We attempt to delineate those differences below by noting if a flag is defaulted (d), set manually (m) or unset (blank):

Flag hipcc amdclang Notes
-O3 d m amdclang is O2 by default
-x hip d m Failure to compile if unset
-D__HIP_HCC_COMPAT_MODE__=1 d m DecisionTree test segfaults when unset
-mrelax-all d
-mframe-pointer=all d
-mframe-pointer=none d
-Wno-format-nonliteral d
-fallow-half-arguments-and-returns d
-mllvm -amdgpu-early-inline-all=true d
-mllvm -amdgpu-function-calls=false d
--genco m
--cuda-device-only d m This is set for hipcc when genco is passed

To reproduce one can do the following:

Configure the HostTestLibrary directory

# hipcc
cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER=hipcc -DTensile_CPU_THREADS=32 -DTensile_ROOT=`pwd`/Tensile -S HostLibraryTests -B build

# amdclang++
cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_COMPILER=/opt/rocm/bin/amdclang++ -DTensile_CPU_THREADS=32 -DTensile_ROOT=`pwd`/Tensile -S HostLibraryTests -B build/host3 -DCMAKE_CXX_FLAGS="-D__HIP_HCC_COMPAT_MODE__=1"

Run the build

cmake --build build/host -j 32

Remove DecisionTree test binaries, rebuild with verbosity and review the history for build command

rm build/host/CMakeFiles/TensileTest.dir/DecisionTree_test.cpp.o
cmake --build build/host --verbose

Run the build command from above with -v

cd build/host
/opt/rocm/bin/amdclang++ -v <args...> CMakeFiles/TensileTests.dir/DecisionTree_test.cpp.o -MF CMakeFiles/TensileTests.dir/DecisionTree_test.cpp.o.d -o CMakeFiles/TensileTests.dir/DecisionTree_test.cpp.o -c /mnt/host/projects/tensile-wts/clang-migration/HostLibraryTests/DecisionTree_test.cpp

The command above will output the detailed compilation commands and arguments used to determine differences in compilation with hipcc and amdclang++.

@ellosel ellosel added the BuildPackaging CMake and other build related enhancements label May 17, 2024
Comment on lines 2302 to 2305
if os.name == "nt":
globalParameters["AssemblerPath"] = locateExe(globalParameters["ROCmBinPath"], "clang++.exe")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about amdclang on windows?

@@ -40,7 +40,7 @@ set(TENSILE_USE_OPENMP ON CACHE BOOL "Use OpenMP to improve performance.")
set(TENSILE_STATIC_ONLY ON CACHE BOOL "Disable exposing Tensile symbols in a shared library.")

if(NOT DEFINED CXX_VERSION_STRING)
if(CMAKE_CXX_COMPILER MATCHES ".*/hipcc$")
if(CMAKE_CXX_COMPILER MATCHES ".*/hipcc$" OR CMAKE_CXX_COMPILER MATCHES ".*clang\\+\\+")
Copy link
Contributor Author

@ellosel ellosel May 21, 2024

Choose a reason for hiding this comment

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

Consider using CMAKE_<LANG>_COMPILER_VERSION (new ticket):

https://cmake.org/cmake/help/v3.0/variable/CMAKE_LANG_COMPILER_VERSION.html

@@ -2230,8 +2231,8 @@ def capRow(caps, cap):
def which(p):
exes = [p+x for x in ['', '.exe', '.bat']]
system_path = os.environ['PATH'].split(os.pathsep)
if p == 'hipcc' and 'CMAKE_CXX_COMPILER' in os.environ and os.path.isfile(os.environ['CMAKE_CXX_COMPILER']):
return os.environ['CMAKE_CXX_COMPILER']
if (p == 'hipcc' or p == 'amdclang++') and 'CXX' in os.environ and os.path.isfile(os.environ['CXX']):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs testing on windows

@@ -466,7 +466,7 @@ def functionPrefix(self, kernel):
kStr += " } while (assumed != old);%s" % (self.endLine)
kStr += "}%s" % (self.endLine)
"""
if globalParameters["CxxCompiler"] == "hipcc":
if globalParameters["CxxCompiler"] == "hipcc" or globalParameters["CxxCompiler"] == "amdclang++":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look into removing OCL ticket

hipccver = globalParameters['HipClangVersion'].split(".")
hipccMaj = int(hipccver[0])
hipccMin = int(hipccver[1])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using amdclang we will always pass the if test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe a ticket for dropping 5.2

@@ -40,7 +40,7 @@ set(TENSILE_USE_OPENMP ON CACHE BOOL "Use OpenMP to improve performance.")
set(TENSILE_STATIC_ONLY ON CACHE BOOL "Disable exposing Tensile symbols in a shared library.")

if(NOT DEFINED CXX_VERSION_STRING)
if(CMAKE_CXX_COMPILER MATCHES ".*/hipcc$")
if(CMAKE_CXX_COMPILER MATCHES ".*/hipcc$" OR CMAKE_CXX_COMPILER MATCHES ".*clang\\+\\+")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this check match version-suffixed compiler names? If it needs that .* at the start to match amdclang++, I think it might need a .* on the end to match clang++-17.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am exploring removing this logic altogether in favor of CMAKE_<LANG>_COMPILER_VERSION

@ellosel ellosel force-pushed the clang-migration branch 3 times, most recently from fe3d527 to a7ef0fc Compare May 29, 2024 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BuildPackaging CMake and other build related enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants