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

Add OpenCL Model #744

Merged
merged 35 commits into from
Oct 14, 2024
Merged

Conversation

tonghaining
Copy link
Contributor

No description provided.

Copy link
Owner

@hernanponcedeleon hernanponcedeleon left a comment

Choose a reason for hiding this comment

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

A few comments based on a quick look to the changes. Will take a closer look next week

Copy link
Owner

@hernanponcedeleon hernanponcedeleon left a comment

Choose a reason for hiding this comment

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

There is something about the overall code that I don't like (I am probably the one the blame about this).

I think the reason we have C and OpenCL things mixed in a bad way now is because we have very few OpenCL tests and we wanted to use the C litmus tests we have to validate OpenCL model: the OpenCL model is an extension of the C model and thus we should get the same results on pure C programs.

We should rather port our C litmus the OpenCL syntax (which is pretty much the same except the syntax will explicitly state the scopes and the address space, avoiding the need to do this magic in the parser).

What about the following:

  • Litmus intended to be run wrt opencl.cat should have OpenCL in the first line
  • Litmus having OpenCL int he first line should have @ for the threads to explicitly state the hierarchy
  • For Litmus having OpenCL in the first line, we add scopes, address spaces, and any other opencl tag which are explicitly stated in the syntax and we default to some specific ones when not explicitly stated. This should make the porting og C litmus so OpenCL easy (just change the first line and add @wg0, dev0 to all tests). Otherwise, we request explicit mention of the needed tags. This might make the porting harder and I am not sure it has any benefit.
  • We use the same visitor for litmus having C and OpenCL in the first line. Most of the visitor methods will be the same, execpt that depending on the first line, we might need to add some tags. But at least this should result in cleaner code about the usage of Thread or ScopedThread

@tonghaining tonghaining force-pushed the opencl1 branch 2 times, most recently from 31f7ff3 to 7821c1d Compare October 2, 2024 15:36
@tonghaining
Copy link
Contributor Author

Sounds good to me.

  • For Litmus having OpenCL in the first line, we add scopes, address spaces, and any other opencl tag which are explicitly stated in the syntax and we default to some specific ones when not explicitly stated. This should make the porting og C litmus so OpenCL easy (just change the first line and add @wg0, dev0 to all tests). Otherwise, we request explicit mention of the needed tags. This might make the porting harder and I am not sure it has any benefit.

One more change we need is to add "global" for all variables for C litmus

@tonghaining tonghaining force-pushed the opencl1 branch 2 times, most recently from ec8b35c to b7318f0 Compare October 5, 2024 07:13
Copy link
Owner

@hernanponcedeleon hernanponcedeleon left a comment

Choose a reason for hiding this comment

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

Comment about the code. I still have not check in detail the cat models and the tests.

cat/c11.cat Outdated Show resolved Hide resolved
cat/c11-partialsc.cat Outdated Show resolved Hide resolved
cat/opencl.cat Outdated Show resolved Hide resolved
cat/opencl.cat Outdated Show resolved Hide resolved
Copy link
Owner

@hernanponcedeleon hernanponcedeleon left a comment

Choose a reason for hiding this comment

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

We are almost there ;)

Please answer the review with new commits (i.e. do not rebase) so it is easier to review the next time (otherwise I need to go over a large diff)

cat/opencl-herd.cat Show resolved Hide resolved
cat/opencl.cat Show resolved Hide resolved
cat/opencl.cat Outdated Show resolved Hide resolved
cat/opencl.cat Outdated Show resolved Hide resolved
cat/opencl.cat Show resolved Hide resolved
cat/opencl.cat Outdated Show resolved Hide resolved
@hernanponcedeleon
Copy link
Owner

Btw ... Don't forget to add opencl as a target in the README

cat/opencl.cat Outdated Show resolved Hide resolved
cat/opencl.cat Outdated Show resolved Hide resolved
cat/opencl.cat Outdated Show resolved Hide resolved
@hernanponcedeleon
Copy link
Owner

Unless @ThomasHaas has more comments, I can merge once the CI passes

@hernanponcedeleon hernanponcedeleon merged commit 253a119 into hernanponcedeleon:development Oct 14, 2024
1 check passed
@tonghaining tonghaining deleted the opencl1 branch October 14, 2024 13:43
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