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

[HIP] Add support for global variable read write #1186

Merged
merged 4 commits into from
Jan 12, 2024

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Dec 13, 2023

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9f14fed) 15.73% compared to head (45d76b7) 15.73%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1186      +/-   ##
==========================================
- Coverage   15.73%   15.73%   -0.01%     
==========================================
  Files         223      223              
  Lines       31478    31477       -1     
  Branches     3558     3558              
==========================================
- Hits         4954     4953       -1     
  Misses      26473    26473              
  Partials       51       51              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

source/adapters/hip/enqueue.cpp Show resolved Hide resolved
source/adapters/hip/enqueue.cpp Outdated Show resolved Hide resolved
source/adapters/hip/enqueue.cpp Outdated Show resolved Hide resolved
source/adapters/hip/program.cpp Show resolved Hide resolved
source/adapters/hip/enqueue.cpp Show resolved Hide resolved
source/adapters/hip/enqueue.cpp Outdated Show resolved Hide resolved
@hdelan hdelan added the ready to merge Added to PR's which are ready to merge label Jan 5, 2024
hdelan and others added 4 commits January 8, 2024 10:38
@hdelan
Copy link
Contributor Author

hdelan commented Jan 8, 2024

Ping @oneapi-src/owner-unified-runtime can we merge this?

@kbenzie
Copy link
Contributor

kbenzie commented Jan 10, 2024

Ping @oneapi-src/owner-unified-runtime can we merge this?

I don't see any intel/llvm testing for this. Since its adding a support for a feature not previously supported by HIP its not NFC so should be tested.

Edit: Of course I notice intel/llvm#12165 it after posting this.

@abagusetty
Copy link

@oneapi-src/owner-unified-runtime, gentle reminder for merge

@aarongreig aarongreig merged commit 79c28d0 into oneapi-src:main Jan 12, 2024
51 checks passed
sarnex pushed a commit to intel/llvm that referenced this pull request Jan 16, 2024
Enable `device_global` for AMDGPU. Related to
oneapi-src/unified-runtime#1186 .

In order for `hipModuleGetGlobal` to see a global symbol we need to not
make the visibility of the symbol hidden. Perhaps changing this as a
default is not always a good idea. Could also do this in the SYCL
headers using an attribute to specify the visibility of the
`device_global` var
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Added to PR's which are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants