----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3185/#review7448 -----------------------------------------------------------
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 (line 203) <http://reviews.gem5.org/r/3185/#comment6296> 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 (line 94) <http://reviews.gem5.org/r/3185/#comment6299> Please keep spacing consistent with other parts of code here. This line is particularly difficult to parse. src/mem/protocol/RubySlicc_Exports.sm (line 176) <http://reviews.gem5.org/r/3185/#comment6300> Why have separate GPU atomic types? I don't see where there are used in other patches. src/mem/request.hh (line 162) <http://reviews.gem5.org/r/3185/#comment6289> This flag doesn't appear to be used anywhere... src/mem/request.hh (line 167) <http://reviews.gem5.org/r/3185/#comment6290> Can you please make this more descriptive? OS kernel or accelerator kernel? Why is it necessary? src/mem/request.hh (line 238) <http://reviews.gem5.org/r/3185/#comment6291> 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 (line 373) <http://reviews.gem5.org/r/3185/#comment6292> Can you please set the functor pointer in the constructor header similar to other Request data members? src/mem/request.hh (line 426) <http://reviews.gem5.org/r/3185/#comment6294> Are all of these new constructors necessary? src/mem/request.hh (line 427) <http://reviews.gem5.org/r/3185/#comment6293> Please follow the gem5 style and name parameters with lower_underscore case. src/mem/request.hh (line 435) <http://reviews.gem5.org/r/3185/#comment6295> Why don't these new constructors initialize member variables like the existing constructors? src/mem/request.hh (line 795) <http://reviews.gem5.org/r/3185/#comment6298> || _flags.isSet(ATOMIC_OP) ???? src/mem/ruby/common/DataBlock.cc (line 66) <http://reviews.gem5.org/r/3185/#comment6301> Whitespace src/mem/ruby/common/DataBlock.cc (line 75) <http://reviews.gem5.org/r/3185/#comment6302> Whitespace src/mem/ruby/common/DataBlock.cc (line 104) <http://reviews.gem5.org/r/3185/#comment6303> How is this function different from getData? src/mem/ruby/common/WriteMask.hh (line 142) <http://reviews.gem5.org/r/3185/#comment6304> Whitespace in this file makes it tough to parse src/mem/ruby/common/WriteMask.hh (line 160) <http://reviews.gem5.org/r/3185/#comment6305> This doesn't appear to match other variable naming style in the file. src/mem/ruby/common/WriteMask.cc (line 39) <http://reviews.gem5.org/r/3185/#comment6306> File does not pass gem5 style checks. Please see http://gem5.org/Coding_Style#Spacing src/mem/ruby/slicc_interface/RubyRequest.hh (line 117) <http://reviews.gem5.org/r/3185/#comment6307> Whitespace src/mem/ruby/slicc_interface/RubyRequest.hh (line 147) <http://reviews.gem5.org/r/3185/#comment6308> Whitespace src/mem/ruby/system/RubyPort.cc (line 498) <http://reviews.gem5.org/r/3185/#comment6309> Please add comments for what this code does. - Joel Hestness On Nov. 2, 2015, 10:43 p.m., Tony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3185/ > ----------------------------------------------------------- > > (Updated Nov. 2, 2015, 10:43 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11191:a831152645c0 > --------------------------- > mem: support for scoped atomics in GPU ruby protocols > > This patch extends ruby to: (1) do write combining, (2) do GPU-style RMWs at > the > memory controller via functors, (3) support HSA-style scoped memory > operations. > > > Diffs > ----- > > src/mem/ruby/common/SConscript 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/common/WriteMask.hh PRE-CREATION > src/mem/ruby/common/WriteMask.cc PRE-CREATION > src/mem/ruby/slicc_interface/RubyRequest.hh > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/slicc_interface/RubySlicc_Util.hh > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/system/RubyPort.cc 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/abstract_mem.cc 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/packet.hh 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/protocol/RubySlicc_Exports.sm > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/base/types.hh 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/SConscript 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/common/DataBlock.hh 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/common/DataBlock.cc 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/protocol/RubySlicc_Types.sm > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/request.hh 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > > Diff: http://reviews.gem5.org/r/3185/diff/ > > > Testing > ------- > > > Thanks, > > Tony Gutierrez > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
