> On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > 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.
Hey Joel, First off, thanks for your review. FYI, please keep in mind that the original authors of this patch, who were an intern and a post-doc, are no longer at AMD. I've been assigned to this patch, and will do my best to address the feedback, but it will take some effort, since I didn't write the code... I've responded to comments below. I'm still trying to decide how to handle the feedback. I'm thinking to: (1) Take care of the style issues and minor feedback you pointed out. (2) Split up the patch as Andreas requested. We will need to copy the unaddressed comments over... (3) Tackle the larger pieces of feedback, after the patches are split up. Thanks, Marc > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/base/types.hh, line 203 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50976#file50976line203> > > > > 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? I think I agree. I will take this on after splitting up the patches, as Andreas has requested. > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/protocol/RubySlicc_Exports.sm, line 94 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50979#file50979line94> > > > > Please keep spacing consistent with other parts of code here. This line > > is particularly difficult to parse. I'll fix this. > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/request.hh, line 238 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50981#file50981line238> > > > > 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? This is dead code. It will be removed. > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/request.hh, line 373 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50981#file50981line373> > > > > Can you please set the functor pointer in the constructor header > > similar to other Request data members? setAtomicOpFunctor is a function, not a variable. It can't be called in the constructor initialization list. > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/protocol/RubySlicc_Exports.sm, line 176 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50979#file50979line176> > > > > Why have separate GPU atomic types? I don't see where there are used in > > other patches. 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. > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/request.hh, line 162 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50981#file50981line162> > > > > This flag doesn't appear to be used anywhere... 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. > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/request.hh, line 167 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50981#file50981line167> > > > > Can you please make this more descriptive? OS kernel or accelerator > > kernel? Why is it necessary? 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. > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/request.hh, line 426 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50981#file50981line426> > > > > Are all of these new constructors necessary? I will follow up on this. I'm not sure. > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/request.hh, line 427 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50981#file50981line427> > > > > Please follow the gem5 style and name parameters with lower_underscore > > case. I will fix this. > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/request.hh, line 435 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50981#file50981line435> > > > > Why don't these new constructors initialize member variables like the > > existing constructors? This comment is redundant --- right? Same thing I said about the initialization list above... > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/request.hh, line 795 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50981#file50981line795> > > > > || _flags.isSet(ATOMIC_OP) ???? I don't understand your comment. > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/ruby/common/DataBlock.cc, line 66 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50984#file50984line66> > > > > Whitespace I will fix this. > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/ruby/common/DataBlock.cc, line 75 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50984#file50984line75> > > > > Whitespace I will fix this. > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/ruby/common/DataBlock.cc, line 104 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50984#file50984line104> > > > > How is this function different from getData? Looks like it's declared without const... It does seem redundant... I will look into this further. > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/ruby/common/WriteMask.hh, line 142 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50986#file50986line142> > > > > Whitespace in this file makes it tough to parse I'll fix this... > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/ruby/common/WriteMask.hh, line 160 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50986#file50986line160> > > > > This doesn't appear to match other variable naming style in the file. Ok. I'll fix this... > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/ruby/common/WriteMask.cc, line 39 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50987#file50987line39> > > > > File does not pass gem5 style checks. Please see > > http://gem5.org/Coding_Style#Spacing Ok. I'll fix this... > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/ruby/slicc_interface/RubyRequest.hh, line 117 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50988#file50988line117> > > > > Whitespace Ok. I'll fix this... > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/ruby/slicc_interface/RubyRequest.hh, line 147 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50988#file50988line147> > > > > Whitespace Ok. I'll fix this... > On Nov. 2, 2015, 11:56 p.m., Joel Hestness wrote: > > src/mem/ruby/system/RubyPort.cc, line 498 > > <http://reviews.gem5.org/r/3185/diff/1/?file=50990#file50990line498> > > > > Please add comments for what this code does. Same comment as above. I'll try to get rid of this. If I can't, I'll comment it. - Marc ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3185/#review7448 ----------------------------------------------------------- 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
