Repository navigation
fix(ocl) - Install ocltst files to bin dir, ocltst libs to paths according to OS - #4305
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts the core-ocl artifact packaging rules to align which OpenCL CLR outputs get shipped in the run vs test components.
Changes:
- Narrow
components.runcontents from allbin/**to justbin/clinfo. - Narrow
components.testcontents fromtests/**andshare/opencl/**to justbin/ocltst.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
includes rpath to opencl build
remove opencl checks in sharedLib tests since we are shipping it as of now
ScottTodd
left a comment
There was a problem hiding this comment.
Generally LGTM. Let's get ROCm/rocm-systems#4632 landed and pulled in so we can drop the rocm-systems submodule change from this PR before merge
| "bin/*oclruntime*", | ||
| "lib/*oclgl*", | ||
| "lib/*oclperf*", | ||
| "lib/*oclruntime*", |
There was a problem hiding this comment.
I pulled down the artifacts to test this:
D:\projects\TheRock (main -> upstream)
(.venv) λ python build_tools/artifact_manager.py fetch ^
--run-id=24252731459 ^
--stage=all ^
--flatten ^
--amdgpu-families=gfx110X-all ^
--output-dir=D:/scratch/therock/24252731459/Do you want to put oclruntime.dll in bin/ on Windows? That would more closely match other subprojects:
>>> paths = list(Path.cwd().glob("bin/*ocl*")) + list(Path.cwd().glob("lib/*ocl*"))
>>> for path in paths:
... print(path)
...
D:\scratch\therock\24252731459\bin\amdocl64.dll
D:\scratch\therock\24252731459\bin\oclperf.exclude
D:\scratch\therock\24252731459\bin\oclruntime.exclude
D:\scratch\therock\24252731459\bin\ocltst.exe
D:\scratch\therock\24252731459\lib\amdocl64.lib
D:\scratch\therock\24252731459\lib\oclperf.dll
D:\scratch\therock\24252731459\lib\oclperf.lib
D:\scratch\therock\24252731459\lib\oclruntime.dll
D:\scratch\therock\24252731459\lib\oclruntime.lib
I was able to run ocltst with the DLL in lib, just a bit awkward:
λ .\ocltst -m ..\lib\oclruntime.dll -A oclruntime.exclude
Compute Device info:
Platform Version: OpenCL 2.1 AMD-APP (3652.0)
Device Name: gfx1100
Vendor: Advanced Micro Devices, Inc.
Device Version: OpenCL 2.0 AMD-APP (3652.0)
Driver Version: 3652.0 (PAL,LC)
Board Name: AMD Radeon PRO W7900 Dual Slot
-------------------------------------------------
The OpenCL Testing Module oclruntime Version = 0
-------------------------------------------------
OCLCreateContext[ 0] PASSED: GPU[0] T[0] [ 0], 0.000
OCLKernelBinary[ 0] PASSED: GPU[0] T[0] [ 0], 0.000
There was a problem hiding this comment.
if you approve this, I can merge the rocm-systems and wait for rocm-systems bump for this PR.
Parallelly can work on fixing the ocltst PR to pick these new path changes
#3590
There was a problem hiding this comment.
The code changes LGTM. Need to merge the rocm-systems PR first and pull that in during a regular submodule update. I'm not going to approve until the submodule update is separate from this PR (we're hemorrhaging on submodule updates lately due to infra issues, build failures, and test failures, and I don't want to risk an unscheduled update slipping through without going through the standard process)
There was a problem hiding this comment.
sure thanks. I merged ROCm/rocm-systems#4632 into rocm-systems.
Will drop rocm-systems from this PR and wait for the rocm-systems bump.
There was a problem hiding this comment.
@ScottTodd - I had to revert ROCm/rocm-systems#4632 because of missing OpenCL.so dependency with liboclperf.so, liboclruntime.so. Opencl.so dependency is satisfied by ubuntu packages ocl-icd-libopencl1, ocl-icd-opencl-dev. This PR skips checking the ldd for these test libs.
New rocm-systems PR - ROCm/rocm-systems#9804
Once you approve , I need to rebase and drop rocm-systems in this PR.
rocm-systems PR depends on this rock PR. So this needs to go in first.
for the folder structure of test files please see below screenshots
Linux
https://therock-ci-artifacts.s3.amazonaws.com/33570960219-linux/core-ocl_test_generic.tar.zst

windows
https://therock-ci-artifacts.s3.amazonaws.com/33570960219-windows/core-ocl_test_generic.tar.zst

There was a problem hiding this comment.
SGTM, will take a look at the new PR.
There was a problem hiding this comment.
I see you approved https://github.com/ROCm/rocm-systems/pull/9804/changes. Will drop rocm-systems in this PR and rebase this. So we can merge this next week
There was a problem hiding this comment.
@ScottTodd - I dropped rocm-systems from this PR and its rebased. All required tests have passed.
Can you help approve this PR please.
There was a problem hiding this comment.
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
…ocltst_path r-s in this PR will be dropped after the testing is complete
|
Windows tests on presubmit were switched to opt-in last week with #8710 due to 48h+ runner queues. I added the opt-in label so we can see test results there :/. Could also trigger the test workflow directly. |
ScottTodd
left a comment
There was a problem hiding this comment.
CI results look complete enough now
…libs to lib dir (#9804) ## Motivation Install ocltst to bin folder for both linux and windows The ocltst libraries need to be installed to bin - on windows lib - on linux ROCm/TheRock#4192 Install ocltst to bin folder for both linux and windows align to packaging requirement. Install ocltst to bin folder for both linux and windows. On windows, this helps to test the runtime libraries without having to do extra copies or setup ## Technical Details Install ocltst to bin folder for both linux and windows The ocltst libraries need to be installed to bin - on windows lib - on linux same change was merged before - #4632 but had to be reverted because of missing Opencl.so issue. Revert commit - 90792ab testing this change in Rock - ROCm/TheRock#4305 ## Issue Tracking <!-- Include a GitHub Issue and/or JIRA ID. Do not post any full JIRA links here. --> <!-- GitHub issue: #1234 --> <!-- JIRA ID: EXAMPLECOMPONENT-1234 --> ROCm/TheRock#4192 ## Test Plan Rock build passes testing this change in Rock - ROCm/TheRock#4305 ## Test Result Rock build passes ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
ocl test libs to lib dir (#9804) ## Motivation Install ocltst to bin folder for both linux and windows The ocltst libraries need to be installed to bin - on windows lib - on linux ROCm/TheRock#4192 Install ocltst to bin folder for both linux and windows align to packaging requirement. Install ocltst to bin folder for both linux and windows. On windows, this helps to test the runtime libraries without having to do extra copies or setup ## Technical Details Install ocltst to bin folder for both linux and windows The ocltst libraries need to be installed to bin - on windows lib - on linux same change was merged before - ROCm/rocm-systems#4632 but had to be reverted because of missing Opencl.so issue. Revert commit - ROCm/rocm-systems@90792ab testing this change in Rock - ROCm/TheRock#4305 ## Issue Tracking <!-- Include a GitHub Issue and/or JIRA ID. Do not post any full JIRA links here. --> <!-- GitHub issue: ROCm/rocm-systems#1234 --> <!-- JIRA ID: EXAMPLECOMPONENT-1234 --> ROCm/TheRock#4192 ## Test Plan Rock build passes testing this change in Rock - ROCm/TheRock#4305 ## Test Result Rock build passes ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. [rocm-systems] ROCm/rocm-systems#9804 (commit 7054b96)
…libs to lib dir (#9804) ## Motivation Install ocltst to bin folder for both linux and windows The ocltst libraries need to be installed to bin - on windows lib - on linux ROCm/TheRock#4192 Install ocltst to bin folder for both linux and windows align to packaging requirement. Install ocltst to bin folder for both linux and windows. On windows, this helps to test the runtime libraries without having to do extra copies or setup ## Technical Details Install ocltst to bin folder for both linux and windows The ocltst libraries need to be installed to bin - on windows lib - on linux same change was merged before - #4632 but had to be reverted because of missing Opencl.so issue. Revert commit - 90792ab testing this change in Rock - ROCm/TheRock#4305 ## Issue Tracking <!-- Include a GitHub Issue and/or JIRA ID. Do not post any full JIRA links here. --> <!-- GitHub issue: #1234 --> <!-- JIRA ID: EXAMPLECOMPONENT-1234 --> ROCm/TheRock#4192 ## Test Plan Rock build passes testing this change in Rock - ROCm/TheRock#4305 ## Test Result Rock build passes ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>


Motivation
Windows -
Linux -
Requires ocltst installation to bin directory in rocm-system PR
ROCm/rocm-systems#4632 - originally merged but was reverted becasue of missing Opencl dependencies in the libs. Opencl will be provided by distro package
ROCm/rocm-systems#9804 - new PR to install the files in the right folder.
This rock PR should independently succeed without changes from rocm-systems. Only rocm-systems PR9804 depends on this PR
ISSUE ID: #4192
Test Plan
ocl_test package should have all files required to execute the tests
Test Result
ocl_test to have the required files
Submission Checklist