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

Windows Support for hip #13

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

dc-dc-dc
Copy link

Added windows support for hip

  • Changed test helper name cuda_compile -> compile
  • Added arg for filename to compile
  • Added LIB_HIP, LIB_HIPRTC constants to gpuctypes/hip.py
  • Changed the library lookup to use a the string constants

@dc-dc-dc
Copy link
Author

wait, is the goal to have the changes in the generate scripts?

@geohot
Copy link
Contributor

geohot commented Nov 26, 2023

I meant to call it cuda_compile, compile is a reserved keyword in Python

@geohot
Copy link
Contributor

geohot commented Nov 26, 2023

Err, and you can't really modify gpuctypes/hip.py, yea, it needs to be in the script.

@dc-dc-dc
Copy link
Author

yeah redoing to inject it from the script

@geohot
Copy link
Contributor

geohot commented Nov 26, 2023

The script injection stuff really needs to be cleaned up if it's going to be more than a one line change

test/helpers.py Outdated Show resolved Hide resolved
test/test_hip.py Outdated

def test_compile(self):
prg = cuda_compile("int test() { return 42; }", ["--offload-arch=gfx1100"], HIPCompile, check)
prg = compile("int test() { return 42; }", ["--offload-arch=gfx1100"], HIPCompile, check, filename=None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Windows isn't okay with <null>?

Copy link
Author

Choose a reason for hiding this comment

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

it would throw an error about the profiler not being initialized, not sure how the filename relates to it tho but can debug when I have some spare time

@geohot
Copy link
Contributor

geohot commented Nov 26, 2023

#14 should make CI fail if the autogeneration doesn't match

@geohot
Copy link
Contributor

geohot commented Nov 26, 2023

The LIB_HIPRTC change also shouldn't matter. The dict name can still be .so on Windows

@dc-dc-dc
Copy link
Author

fixed CI error

if 'linux' in sys.platform:
return ctypes.CDLL(os.path.join('/opt/rocm/lib/libhiprtc.so'))
elif 'win' in sys.platform:
hip_path = os.getenv('HIP_PATH', None)
Copy link
Contributor

@geohot geohot Nov 29, 2023

Choose a reason for hiding this comment

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

Is there a way to not need this? How come it can find amdhip64?

Copy link
Author

Choose a reason for hiding this comment

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

amdhip64 gets installed in System32 where cdll searches for dlls, the rtc does not so have to use HIP_PATH to find the hip installation. HIP_PATH is added to the env on install of the hip sdk

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