I'm not blown away by the description... :-)
Could you please explain what the patch is accomplishing, and the concept/idea behind the implementation. From a quick glance it seems very invasive on the memory system, and seems to mix a number of different pieces of functionality.

This is pretty cool. Looking forward to when gem5-gpu can shift over to use this!
A couple big questions, but mostly minor comments below.

src/base/types.hh (Diff revision 1)

I'm a little confused about the need for this. Is it not possible to just set a function pointer in Requests? Why do we need to dynamically instantiate and delete a wrapper class for the function pointer in each atomic Request?

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   I think I agree. I will take this on after splitting up the patches, as Andreas has requested.

src/mem/protocol/RubySlicc_Exports.sm (Diff revision 1)

Please keep spacing consistent with other parts of code here. This line is particularly difficult to parse.

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   I'll fix this.

src/mem/protocol/RubySlicc_Exports.sm (Diff revision 1)

Why have separate GPU atomic types? I don't see where there are used in other patches.

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   Good question... I was just looking at this. HSA defines two types of atomics: RETURN and NO_RETURN. RETURN atomics actually return a value (e.g., compare-and-swap or fetch-and-add). NO_RETURN do not return values (e.g., atomic_increment).
   Anyways, it seems like we've got the plumbing in place to handle NO_RETURN, but currently treat all atomics as RETURN. I think we haven't been bitten by this, because all of our benchmarks that we've used only use return atomics, or if no_return occur, it was harmless to actually copy the value to a destination register.
   In any case, I think that it should be trivial to update the patch to handle these two cases properly. So, currently, that's what I'm thinking to do.

src/mem/request.hh (Diff revision 1)

This flag doesn't appear to be used anywhere...

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   Good catch. After looking at it, we should either use this flag, or the RETURN/NO_RETURN, described above. Currently, I'm leaning towards getting rid of this one, but I will make a final decision when I actually update/test the patch.

src/mem/request.hh (Diff revision 1)

Can you please make this more descriptive? OS kernel or accelerator kernel? Why is it necessary?

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   It is to is flush/invalidate GPU caches on accelerator kernel launch / end.
   I want to get rid of this actually. It seems redundant to acquire/release synchronization to me. I will try, but if I can't get things working without it, I will improve the comments.

src/mem/request.hh (Diff revision 1)

I understand that 'visibility' may refer to whether particular work-items should have visibility to requests from other work-items. However, the OpenCL spec lists visibility as a property of the memory to which a request is being sent rather than a property of requests themselves. I don't see that visibility is ever used in your other patches. Is there something more that visibility represents that cannot be represented by the scope and target memory type in these ExtraFlags?

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   This is dead code. It will be removed.

src/mem/request.hh (Diff revision 1)

Can you please set the functor pointer in the constructor header similar to other Request data members?

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   setAtomicOpFunctor is a function, not a variable. It can't be called in the constructor initialization list.

src/mem/request.hh (Diff revision 1)

Are all of these new constructors necessary?

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   I will follow up on this. I'm not sure.

src/mem/request.hh (Diff revision 1)

Please follow the gem5 style and name parameters with lower_underscore case.

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   I will fix this.

src/mem/request.hh (Diff revision 1)

Why don't these new constructors initialize member variables like the existing constructors?

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   This comment is redundant --- right? Same thing I said about the initialization list above...

src/mem/request.hh (Diff revision 1)

|| _flags.isSet(ATOMIC_OP) ????

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   I don't understand your comment.

src/mem/ruby/common/DataBlock.cc (Diff revision 1)

Whitespace

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   I will fix this.

src/mem/ruby/common/DataBlock.cc (Diff revision 1)

Whitespace

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   I will fix this.

src/mem/ruby/common/DataBlock.cc (Diff revision 1)

How is this function different from getData?

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   Looks like it's declared without const... It does seem redundant... I will look into this further.

src/mem/ruby/common/WriteMask.hh (Diff revision 1)

Whitespace in this file makes it tough to parse

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   I'll fix this...

src/mem/ruby/common/WriteMask.hh (Diff revision 1)

This doesn't appear to match other variable naming style in the file.

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   Ok. I'll fix this...

src/mem/ruby/common/WriteMask.cc (Diff revision 1)

File does not pass gem5 style checks. Please see http://gem5.org/Coding_Style#Spacing

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   Ok. I'll fix this...

src/mem/ruby/slicc_interface/RubyRequest.hh (Diff revision 1)

Whitespace

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   Ok. I'll fix this...

src/mem/ruby/slicc_interface/RubyRequest.hh (Diff revision 1)

Whitespace

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   Ok. I'll fix this...

src/mem/ruby/system/RubyPort.cc (Diff revision 1)

Please add comments for what this code does.

Show all issues

1. Marc Orr Nov. 6, 2015, 11:11 a.m. (Nov. 6, 2015, 11:11 a.m.)

   Same comment as above. I'll try to get rid of this. If I can't, I'll comment it.

In my view this patch really needs to be split:


The adding of write masks is an example of functionality that definitely warrants its own patch, and I would argue the write mask should go into the packet class.


There are a bunch of changes/flags to the request that seem unrelated to the atomics and should be split out.


Finally there are the atomic ops.

src/mem/request.hh (Diff revision 1)

Disregard my prior comment. I see what you mean now.

Show all issues

I think we addressed all comments. We'd like to ship this by end of this week.

src/mem/request.hh (Diff revision 4)

HSA in the name somehow?

Show all issues

1. Joel Hestness Dec. 24, 2015, 10:12 a.m. (Dec. 24, 2015, 10:12 a.m.)

   I'm not sure if it makes sense to use "HSA" here, since "HSA" is meant very broadly to describe the system, while these are specific parameters of the memory spaces/hierarchy (e.g. we'll probably use these if/when we merge in GPGPU-Sim with an NVIDIA-like GPU). However, I agree it would be nice if these flags were named something more descriptive, since "ExtraFlags" is pretty obtuse. Maybe something like "MemHierParamFlags", or "MemSpaceConfigFlags"?

2. Tony Gutierrez Dec. 24, 2015, 11:28 a.m. (Dec. 24, 2015, 11:28 a.m.)

   I agree, I will change the naming before shipping. I prefer MemSpaceConfigFlags out of the two options you propose.

src/mem/request.hh (Diff revision 4)

HSA specific?

Show all issues

src/mem/request.hh (Diff revision 4)

It is somewhat unfortunate how many extra functions we end up with, given the limited use. Could we at least wrap all these in an HSA comment section so that it is clear that they are really only used by the APU?

Show all issues

1. Tony Gutierrez Dec. 24, 2015, 11:28 a.m. (Dec. 24, 2015, 11:28 a.m.)

   Yes. I can certainly do that before shipping.

src/mem/request.hh (Diff revision 4)

I'm still not sure I understand what this means. Why does a request need to be marked to indicate a kernel launch? I can understand if stale caches should be flushed (release) at the beginning or end of a kernel, but this should really be clearer.

Show all issues

1. Tony Gutierrez Dec. 24, 2015, 11:54 a.m. (Dec. 24, 2015, 11:54 a.m.)

   Yes, there is a distinction between regular acq/rel and kernel begin/end. Kernel begin/end are acq/rel respectively with the additional KERNEL flag. They are used to invalidate the caches on a kernel launch, and to writeback the caches on a kernel end. They are also used to indicate to the CU itself to launch/end a kernel when it receives a response. Is that clear? If so I can add a comment to that effect.

2. Joel Hestness Dec. 28, 2015, 10:51 a.m. (Dec. 28, 2015, 10:51 a.m.)

   Cool, thanks for clarifying. That's much clearer, and it would be nice to get this info into the comments.

There seems b

src/mem/protocol/RubySlicc_Exports.sm (Diff revision 5)

seems unrelated

Show all issues

1. Brad Beckmann Jan. 13, 2016, 2:59 p.m. (Jan. 13, 2016, 2:59 p.m.)

   These functions are modified an used by patch 3206.  Ideally this change should've been posted as apart of the dirty-bit-per-word support for ruby patch.  However, unless you strenously object, will check it in as is so that we can move on to higher priority tasks.

src/mem/protocol/RubySlicc_Exports.sm (Diff revision 5)

unrelated?

Show all issues

1. Brad Beckmann Jan. 13, 2016, 2:59 p.m. (Jan. 13, 2016, 2:59 p.m.)

   Sure we can remove this line.

src/mem/protocol/RubySlicc_Exports.sm (Diff revision 5)

This seems unrelated

Show all issues

1. Brad Beckmann Jan. 13, 2016, 2:59 p.m. (Jan. 13, 2016, 2:59 p.m.)

   Similar to above, these functions are modified an used by patch 3206.  Ideally this change should've been posted as apart of the dirty-bit-per-word support for ruby patch.  However, unless you strenously object, will check it in as is so that we can move on to higher priority tasks.

src/mem/protocol/RubySlicc_Types.sm (Diff revision 5)

seems orthogonal

Show all issues

1. Brad Beckmann Jan. 13, 2016, 2:59 p.m. (Jan. 13, 2016, 2:59 p.m.)

   Orthogonal?  These are used below in RubyReqeust.

src/mem/request.hh (Diff revision 5)

Where do these come from? Is there a spec somewhere? A pointer would be good.

Show all issues

1. Brad Beckmann Jan. 13, 2016, 2:59 p.m. (Jan. 13, 2016, 2:59 p.m.)

   Yes, http://hsafoundation.com.  I recommend not including the URL in the code though.  It just causes extra hassle when Legal teams scan the code.

src/mem/request.hh (Diff revision 5)

Given that these flags are all very specific and used by a single module, is it worth holding off on all the getters and simply rely on the getMemSpaceConfigFlags()?

Show all issues

1. Brad Beckmann Jan. 13, 2016, 2:59 p.m. (Jan. 13, 2016, 2:59 p.m.)

   Why push back on this?  These getters make our code easier to read and the cost is minimal to those who don't use them.

src/mem/ruby/system/RubyPort.cc (Diff revision 5)

This seems unrelated

Show all issues

1. Brad Beckmann Jan. 13, 2016, 2:59 p.m. (Jan. 13, 2016, 2:59 p.m.)

   Since you asked us to split the patch, there is not an obvious place to put the code.  We would prefer not to split it, since it is a very simple change.

2. Marc Orr Jan. 13, 2016, 4:46 p.m. (Jan. 13, 2016, 4:46 p.m.)

   To me, this change is ok here. It is a miscellaneous change of the flags for the AMD GPU model. That said, other options would be to commit it as separate changeset or fold into the big GPU patch.

src/mem/ruby/system/RubyPort.cc (Diff revision 5)

unrelated

Show all issues

1. Brad Beckmann Jan. 13, 2016, 2:59 p.m. (Jan. 13, 2016, 2:59 p.m.)

   Again this small change had to be placed somewhere and there was not an obvious place to put it after you asked us to split the code.

There seems to be a few unrelated changes here.

src/mem/request.hh (Diff revision 5)

Looking at the APU/GPU patch, it seems these are more mutually exclusive options than flags in bitmask. Would it not make sense to rather have an enum for the segment, and one for the scope, and then use the same enum here and where the value is acted upon?

Show all issues