Add OpenCL Model #744
Add OpenCL Model #744
Conversation
| @@ -24,6 +25,7 @@ public Program parse(File file) throws Exception { | |||
| if (needsClang(file)) { | |||
| file = compileWithClang(file, ""); | |||
| file = applyLlvmPasses(file); | |||
| file = applyDemangling(file); | |||
There was a problem hiding this comment.
Probably not yet needed. If the PR is just about litmus, please remove everything that is not related
There was a problem hiding this comment.
It is still there. You should also remove the method in Compilation.java (at probably all associated changes to that class)
There was a problem hiding this comment.
Both are removed now
| import static com.dat3m.dartagnan.program.event.EventFactory.*; | ||
| import static com.dat3m.dartagnan.program.event.EventFactory.OpenCL.newOpenCLBarrier; | ||
|
|
||
| public class VisitorOpenCL extends VisitorBase { |
There was a problem hiding this comment.
Do we really need a new visitor? At first glance the only difference with the c11 one is the visitor method for the control barrier and how events are tagged. I feel those could be moved to the c11 one, or at least make this visitor extend the c11 one and thus avoiding having the reimplement some of the event visitors
There was a problem hiding this comment.
Moved into C11 visitor.
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
| // To be compatible with reads from uninit memory, we use our default definition of fr | ||
| //let fr = rf^-1 ; mo |
There was a problem hiding this comment.
Without init events, the initial memory is neither tagged NONATOMIC nor NON_ATOMIC_LOCATION. I think this might cause issues.
dartagnan/src/main/java/com/dat3m/dartagnan/program/event/Tag.java
Outdated
Show resolved
Hide resolved
tonghaining
force-pushed
the
opencl1
branch
from
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.
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
ThreadorScopedThread
| private final Map<Register, MemoryObject> reg2LocMap = new HashMap<>(); | ||
| private final Map<Integer, Map<String, IntegerType>> id2RegTypeMap = new HashMap<>(); | ||
| private final Map<Integer, Map<String, Expression>> id2RegConstMap = new HashMap<>(); | ||
| private final Map<Integer, Map<String, String>> id2RegLocPtrMap = new HashMap<>(); | ||
| private final Map<Integer, Map<String, String>> id2RegLocValMap = new HashMap<>(); |
There was a problem hiding this comment.
I don't really understand what all these are for, but I am pretty sure the code related to this can be simplified quite a bit. For example, why do you need to put a bunch of stuff in maps and the call the initializers? Why not calling initializers directly.
There are also some new methods related to types that look suspicious, specially since it seems you are always using archType
|
Sounds good to me.
One more change we need is to add "global" for all variables for C litmus |
No description provided.