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

Use the cmake variable HALF_INCLUDE_DIR to find half.hpp #3170

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

trixirt
Copy link

@trixirt trixirt commented Jul 29, 2024

On Fedora half.hpp is installed in the system /usr/include dir The use assumes it is installed in half/half.hpp and looks for this path in the cmake configury. Instead of assuming the path has a half/, pass the found half.hpp path as an include argument -I ${HALF_INCLUDE_DIR}

convert all uses of half/half.hpp with a sed script for f in find . -type f -name '*.hpp' -o -name '*.cpp' ; do
sed -i -e 's@#include <half/half.hpp>@#include <half.hpp>@' $f
done

Fixes this issue that was incorrectly closed
#2672

@junliume
Copy link
Collaborator

junliume commented Aug 1, 2024

@trixirt In order to make this change complete, we need to update the submodule MIFin too: https://github.com/search?q=repo%3AROCm%2FMIFin%20half&type=code

Actually you can go into ${MIOpen}/fin and pull the submodule, then make necessary changes there.

@trixirt
Copy link
Author

trixirt commented Aug 1, 2024

There are other instances in the ROCm stack that need this fix.
Can you explain why this change to MIFin would need to happen as a prerequisite ?
MIFin project does not have a Readme, what is it used for ?

@trixirt
Copy link
Author

trixirt commented Aug 2, 2024

See here
ROCm/MIFin#104

@shurale-nkn
Copy link
Contributor

@atamazov

@atamazov
Copy link
Contributor

atamazov commented Aug 7, 2024

@junliume @trixirt @shurale-nkn I suspect this is overkill, because ROCm does not officially support Fedora, https://rocm.docs.amd.com/en/latest/compatibility/compatibility-matrix.html.

On Fedora half.hpp is installed in the system /usr/include

There are other instances in the ROCm stack that need this fix...

Maybe suggesting the simplest workaround for the Fedora users is enough: put a copy of half.hpp into /usr/include/half/. It does not require any modifications in MIOpen, Fin and other components of the ROCm stack.

Otherwise, no objections.

CMakeLists.txt Outdated
message(STATUS "HALF_INCLUDE_DIR: ${HALF_INCLUDE_DIR}")
string(APPEND CMAKE_CXX_FLAGS " -I${HALF_INCLUDE_DIR}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

@trixirt @atamazov I am not sure if I like this. I hope that we do not need to modify CMAKE_CXX_FLAGS in such a way even though we are only appending to it.

@trixirt
Copy link
Author

trixirt commented Sep 4, 2024

@junliume @trixirt @shurale-nkn I suspect this is overkill, because ROCm does not officially support Fedora, https://rocm.docs.amd.com/en/latest/compatibility/compatibility-matrix.html.

On Fedora half.hpp is installed in the system /usr/include

There are other instances in the ROCm stack that need this fix...

Maybe suggesting the simplest workaround for the Fedora users is enough: put a copy of half.hpp into /usr/include/half/. It does not require any modifications in MIOpen, Fin and other components of the ROCm stack.

Otherwise, no objections.

This thrashes the location of the header and causes confusion for the users of the header. The AMD version of half itself does this, the older version installed it to opt/rocm/include, the newer opt/rocm/include/half.

On Fedora half.hpp is installed in the system /usr/include dir
The use assumes it is installed in half/half.hpp and looks for this path
in the cmake configury.  Instead of assuming the path has a half/, pass the
found half.hpp path as an include argument -I ${HALF_INCLUDE_DIR}

convert all uses of half/half.hpp with a sed script
for f in `find . -type f -name '*.hpp' -o -name '*.cpp' `; do
    sed -i -e 's@#include <half/half.hpp>@#include <half.hpp>@' $f
done

Signed-off-by: Tom Rix <[email protected]>
@trixirt
Copy link
Author

trixirt commented Sep 4, 2024

Removed the global change to cxx flags and am using target_include_directories similar to MIFin
Changed signoff to new email account.

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.

4 participants