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

GPU: Add SMatrixGPU and Fwd decls #5425

Merged
merged 2 commits into from Feb 11, 2021

Conversation

mconcas
Copy link
Collaborator

@mconcas mconcas commented Feb 9, 2021

Also replaces the inclusion in DCAfitterN.h

@mconcas mconcas requested review from davidrohr, shahor02 and a team as code owners February 9, 2021 17:42
@mconcas
Copy link
Collaborator Author

mconcas commented Feb 9, 2021

@davidrohr I set up a possible implementation, did I get correctly the approach?
No GPU test included yet.

davidrohr
davidrohr previously approved these changes Feb 10, 2021
Copy link
Collaborator

@davidrohr davidrohr left a comment

Choose a reason for hiding this comment

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

Looks good to me. Do you want to do more tests, or shall I merge?

@mconcas
Copy link
Collaborator Author

mconcas commented Feb 10, 2021

Looks good to me. Do you want to do more tests, or shall I merge?

It actually looks like we have an issue with OpenCL or it is something unrelated?

@davidrohr
Copy link
Collaborator

Argh, indeed, I hadn't checked the CI yet :(
Indeed, it seems to have a problem with the function pointer.
It might actually be that this is already fixed in clang 12, since I have reported it to them some time ago.
But to upgrade, I'd have to create a new GPU CI container with a build of the current clang trunk (which I have to do anyway for other reasons).
Could you perhaps for now add an #ifndef __OPENCL__ for the include you put in the cartesian.h, such that it is not seen in OpenCL? I'll follow up this OpenCL issue in the meantime.

@mconcas
Copy link
Collaborator Author

mconcas commented Feb 10, 2021

Argh, indeed, I hadn't checked the CI yet :(
Indeed, it seems to have a problem with the function pointer.
It might actually be that this is already fixed in clang 12, since I have reported it to them some time ago.
But to upgrade, I'd have to create a new GPU CI container with a build of the current clang trunk (which I have to do anyway for other reasons).
Could you perhaps for now add an #ifndef __OPENCL__ for the include you put in the cartesian.h, such that it is not seen in OpenCL? I'll follow up this OpenCL issue in the meantime.

Yes, I can for sure.
I investigated a bit and it looks like passing the funciton is forbidden in OpenCL 1.2.
Are we so lucky that they improved with 2.0 and the error is triggered just by the flag #pragma OPENCL EXTENSION __cl_clang_function_pointers : enable not taken into account?

Just to understand whether it will be required some changes on the code in near future.

Thanks, setting up the workaround in the meanwhile...

@davidrohr
Copy link
Collaborator

Yes, clang has this extension for function pointers (although it does not work with all types of function pointers, but I guess yours are fine. We'll see). I'll check with clang master, if it fails there, you'd need to change the code indeed, but let's hope not.

@mconcas
Copy link
Collaborator Author

mconcas commented Feb 11, 2021

@davidrohr seems to me that error is not related to this pr. Should I fix it here? Not a problem to me.

@davidrohr
Copy link
Collaborator

Ah, that is my standalone benchmark, which can be compiled after I updated the GPU CI container.
Beforehand, some libs were missing in the container, so before the CI never compiled it, thus never saw the violations.
I'll take care of them.

@davidrohr davidrohr merged commit da5d8f1 into AliceO2Group:dev Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants