-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add OpenCL Model #744
Conversation
There was a problem hiding this 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
dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/ProgramParser.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/compilation/VisitorOpenCL.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/compilation/VisitorOpenCL.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/CoreCodeVerification.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/memory/MemoryObject.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/event/Tag.java
Outdated
Show resolved
Hide resolved
215b2fb
to
a63dd9c
Compare
dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/visitors/VisitorLitmusC.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/visitors/VisitorLitmusC.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
orScopedThread
dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/utils/ProgramBuilder.java
Outdated
Show resolved
Hide resolved
31f7ff3
to
7821c1d
Compare
Sounds good to me.
One more change we need is to add "global" for all variables for C litmus |
ec8b35c
to
b7318f0
Compare
There was a problem hiding this 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.
dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/utils/ProgramBuilder.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/Thread.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/compilation/VisitorC11.java
Show resolved
Hide resolved
dartagnan/src/test/java/com/dat3m/dartagnan/comparison/AbstractComparisonTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Hernan Ponce de Leon <zeta96@gmail.com>
b2cec69
to
3003967
Compare
There was a problem hiding this 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)
dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/visitors/VisitorLitmusC.java
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/visitors/VisitorLitmusC.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/parsers/program/visitors/VisitorLitmusC.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/event/EventFactory.java
Outdated
Show resolved
Hide resolved
Btw ... Don't forget to add opencl as a target in the README |
dartagnan/src/main/java/com/dat3m/dartagnan/program/processing/MemoryAllocation.java
Show resolved
Hide resolved
Unless @ThomasHaas has more comments, I can merge once the CI passes |
dartagnan/src/main/java/com/dat3m/dartagnan/program/event/EventFactory.java
Outdated
Show resolved
Hide resolved
dartagnan/src/main/java/com/dat3m/dartagnan/program/event/core/ControlBarrier.java
Outdated
Show resolved
Hide resolved
253a119
into
hernanponcedeleon:development
No description provided.