src/gpu-compute/wavefront.hh (Diff revision 1)

Surely one of these should be const

Show all issues

1. Andreas Hansson Aug. 4, 2016, 12:44 p.m. (Aug. 4, 2016, 12:44 p.m.)

   I do not see this being addressed. getContext should be const I would imagine.

2. Alexandru Dutu Aug. 12, 2016, 9:10 a.m. (Aug. 12, 2016, 9:10 a.m.)

   I see, you meant the functions to be const. Well the functions can't actually be const as both of them are calling non-const functions (i.e. remap and emplace_back).

3. Andreas Hansson Aug. 15, 2016, 12:27 a.m. (Aug. 15, 2016, 12:27 a.m.)

   It does seem rather alarming that a "getter" is non const. It would be good to either fix this issue, or make it clear what it is really changing.

src/gpu-compute/wavefront.cc (Diff revision 1)

Not too many bonus points for comments here...that's for sure.

Show all issues

1. Alexandru Dutu June 30, 2016, 11:05 a.m. (June 30, 2016, 11:05 a.m.)

   I have added a comment in the header file which should describe the behaviour of this method and added comments inside it where things are not straight forward (i.e. recovergence stack and LDS). Please let me know what is not clear and requires a comment, would be happy to add some more.

2. Andreas Hansson Aug. 4, 2016, 12:44 p.m. (Aug. 4, 2016, 12:44 p.m.)

   I find the code rather unfortunate. It would have been great to see this implemented using e.g. protobuf or similar, but it's up to you if you really think this is maintainable (and understandable).

3. Alexandru Dutu Aug. 12, 2016, 9:10 a.m. (Aug. 12, 2016, 9:10 a.m.)

   Actually, I had 2 implementations for this: 1 using iostreams and the good old void * implementation you see currently. I've chosen this implementation for simplicity as there isn't much difference (in maintainability of readability) between "*(int *)iter = wfId; iter += sizeof(wfId);" and "ProtoMessage::PacketHeader out; out.set_wf_id(w->wfId); ... out->write(wavefront_context);". Meaning one still has to set all the attributes of the object to be serialized. I will be using protobufs more and I might change this code in the future if there is an advantage. Also, the reconvergence stack makes the use of protobufers less convenient as you still have to iterate over it and have a function call to set each entry.

src/gpu-compute/wavefront.cc (Diff revision 1)

Why the custom format? Why not stick to gem5's established methods for serializing and deserializing scalars and containers?

Show all issues

1. Alexandru Dutu June 30, 2016, 11:05 a.m. (June 30, 2016, 11:05 a.m.)

   Andreas, are you refering to the Serializable class? If so, my understanding is that Serializable is used in the context of creating a Checkpoint, therefore it includes the names of the classes and class members in the serialized stream. This something which is not desired in our case as we want the serialized context to be something available to the actual simulated hardware and not just to the simulator. It seems that even SERIALIZE_CONTAINER calls arrayParamOut which includes names in the output stream. Serializable seems quite heavy weight, as it includes this extra class for handling checkpoints (ScopedCheckpointSection) and it does not do what we need.

2. Andreas Hansson July 1, 2016, 1:56 a.m. (July 1, 2016, 1:56 a.m.)

   Ok, then I really do not understand what the patch is trying to do :-). Perhaps you can explain the use-case in the description?

3. Alexandru Dutu July 7, 2016, 1:22 p.m. (July 7, 2016, 1:22 p.m.)

   The functionality which we want to enable is context switching, so this patch adds the methods that get the context of a wavefront ready to be written to the simulated memory. I will update the description.

Ship It!