mem: misc flags for AMD gpu model
Review Request #3185 - Created Oct. 30, 2015 and submitted
| Information | |
|---|---|
| Tony Gutierrez | |
| gem5 | |
| default | |
| Reviewers | |
| Default | |
Changeset 11279:e858f2256ccb
---------------------------
mem: misc flags for AMD gpu modelThis patch add support to mark memory requests/packets with attributes defined
in HSA, such as memory order and scope.
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.
Description: |
|
|---|
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?
-
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.
-
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.
-
src/mem/request.hh (Diff revision 1) -
This flag doesn't appear to be used anywhere...
-
src/mem/request.hh (Diff revision 1) -
Can you please make this more descriptive? OS kernel or accelerator kernel? Why is it necessary?
-
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?
-
src/mem/request.hh (Diff revision 1) -
Can you please set the functor pointer in the constructor header similar to other Request data members?
-
src/mem/request.hh (Diff revision 1) -
Are all of these new constructors necessary?
-
src/mem/request.hh (Diff revision 1) -
Please follow the gem5 style and name parameters with lower_underscore case.
-
src/mem/request.hh (Diff revision 1) -
Why don't these new constructors initialize member variables like the existing constructors?
-
src/mem/request.hh (Diff revision 1) -
|| _flags.isSet(ATOMIC_OP) ????
-
src/mem/ruby/common/DataBlock.cc (Diff revision 1) -
Whitespace
-
src/mem/ruby/common/DataBlock.cc (Diff revision 1) -
Whitespace
-
src/mem/ruby/common/DataBlock.cc (Diff revision 1) -
How is this function different from getData?
-
src/mem/ruby/common/WriteMask.hh (Diff revision 1) -
Whitespace in this file makes it tough to parse
-
src/mem/ruby/common/WriteMask.hh (Diff revision 1) -
This doesn't appear to match other variable naming style in the file.
-
src/mem/ruby/common/WriteMask.cc (Diff revision 1) -
File does not pass gem5 style checks. Please see http://gem5.org/Coding_Style#Spacing
-
src/mem/ruby/slicc_interface/RubyRequest.hh (Diff revision 1) -
Whitespace
-
src/mem/ruby/slicc_interface/RubyRequest.hh (Diff revision 1) -
Whitespace
-
src/mem/ruby/system/RubyPort.cc (Diff revision 1) -
Please add comments for what this code does.
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.
Summary: |
|
||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+315 -31) |
Description: |
|
|||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+250 -27) |
Description: |
|
|||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+250 -27) |
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?
-
src/mem/request.hh (Diff revision 4) -
HSA specific?
-
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?
-
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.
Description: |
|
|||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+254 -27) |
There seems b
-
src/mem/protocol/RubySlicc_Exports.sm (Diff revision 5) -
seems unrelated
-
src/mem/protocol/RubySlicc_Exports.sm (Diff revision 5) -
unrelated?
-
src/mem/protocol/RubySlicc_Exports.sm (Diff revision 5) -
This seems unrelated
-
src/mem/protocol/RubySlicc_Types.sm (Diff revision 5) -
seems orthogonal
-
src/mem/request.hh (Diff revision 5) -
Where do these come from? Is there a spec somewhere? A pointer would be good.
-
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()?
-
src/mem/ruby/system/RubyPort.cc (Diff revision 5) -
This seems unrelated
-
src/mem/ruby/system/RubyPort.cc (Diff revision 5) -
unrelated
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?
