> On Jan. 13, 2016, 10:47 p.m., Andreas Hansson wrote:
> > ...and yes, please split the patch to make it reviewable (if that is a
> > word).
>
> Brad Beckmann wrote:
> Andreas, we simply do not have the time and resources to do so. We have
> spent a lot of time addressing Joel's changes and we have and will address
> the comments made by you and Andreas Sandberg. I hope you can understand
> that we cannot justify more work.
>
> Also please note that there is a precedent for large features coming in
> as one large changeset with minimal reviews. Specifically Ruby was added to
> m5 in one large patch 6145:15cca6ab723a and it never went through any reviews.
>
> Joel Hestness wrote:
> While I'm all for getting this GPU code in soon and with minimal more
> changes, we should be careful with precedent: addition of Ruby (2009)
> pre-dates the use of Reviewboard for gem5 (2010)...
>
> Andreas Hansson wrote:
> The patch is 58k lines. This is not reviewable in its current form.
>
> Andreas Sandberg wrote:
> Brad, I understand that you have spent a lot of time on preparing and
> curating this patch. I'm not asking you to split it into logical commits that
> gradually build up the GPU functionality, that's clearly not feasible at this
> point. What I would like you to do is to create one or more separate review
> requests for the stuff that /isn't/ in the gpu_compute directory. You
> generally post good quality code, so I'm going to assume that the main part
> of GPU code is OK. However, I would still like to review the parts that
> change existing code and or affect common infrastructure.
>
> Brad Beckmann wrote:
> I'm having a real hard time understanding why you want to review the code
> outside the gpu_compute directory. Beyond adding the gpu_compute files, this
> patch only modifies Ruby code other than the top level SConstruct file that
> you have already commented on. There are no changes to "common
> infrastructure" in this patch. Those changes were in our other patches that
> you have already meticulously reviewed. Also to my understanding, no one at
> ARM uses Ruby. Similarly, most of us at AMD rarely use the Classic memory
> system and thus *often* allow ARM changes to those files without reviews from
> AMD. We understand that you will benefit tremendously by getting your code
> checked (rather than maintaining it internally) and there is little cost to
> us, since we do not use the Classic model. We were expecting you would
> reciprocate that policy.
>
> Furthermore, this patch has been on reviewboard for 11 weeks. We've
> worked hard conforming to all of the comments so far. I cannot justify any
> more work on these current set of patches.
>
> Joel Hestness wrote:
> As a third party, I'm with Brad on this one. Of the 58k lines in this
> patch, there are only on the order of 400 lines that were added/modified in
> existing files. These are not huge changes, and we've all already commented
> on them. If you wanted to review the added files more meticulously, you could
> have pulled down the diff instead of fighting with Review Board.
>
> Andreas Hansson wrote:
> The review from Andreas Sandberg on Nov. 10 2015 clearly states that the
> patch is virtually impossible to review in its current form. We were
> expecting that to be addressed first, so that the actual reviewing could
> proceed.
>
> The whole point of reviews is to improve and refine suggested changes,
> not gate them. I most certainly welcome any scrutiny and feedback on any
> patches I have posted. Brad mentions Ruby as an example, and unsurprinsingly
> Ruby has the poorest comment ratio in the entire code-base, and the by far
> the majority of issues identified by code analysers...
>
> Steve Reinhardt wrote:
> Hi Andreas, it turns out that all the changes that are not in
> src/gpu-compute or in Ruby (src/mem/{protocol,ruby,slicc}) are on the first
> page of the diff. If you want to review the Ruby changes (i.e., everything
> that's not in src/gpu-compute and is not a new file), that's all on pages 7
> and 8 of the diff. So I think you can get the same effect as splitting the
> patch just by ignoring pages 2-6 (and most of 7, actually). And if you're
> willing to ignore what's going on inside of Ruby, you only have to look at
> the first page of the diff.
I had quick look at the files outside of the GPU directory (ReviewBoard seems
much snappier now, it didn't work at all last time I tried) and it seems OK to
me.
- Andreas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3189/#review7882
-----------------------------------------------------------
On Jan. 13, 2016, 9:25 p.m., Tony Gutierrez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3189/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 9:25 p.m.)
>
>
> Review request for Default.
>
>
> Repository: gem5
>
>
> Description
> -------
>
> Changeset 11282:118397102cfe
> ---------------------------
> gpu: AMD's baseline GPU model
>
> This patch includes the entire baseline GPU model from AMD.
>
>
> Diffs
> -----
>
> src/gpu-compute/hsa_object.cc PRE-CREATION
> src/gpu-compute/hsail_code.hh PRE-CREATION
> src/gpu-compute/hsail_code.cc PRE-CREATION
> src/gpu-compute/kernel_cfg.hh PRE-CREATION
> src/gpu-compute/kernel_cfg.cc PRE-CREATION
> src/gpu-compute/lds_state.hh PRE-CREATION
> src/gpu-compute/lds_state.cc PRE-CREATION
> src/gpu-compute/local_memory_pipeline.hh PRE-CREATION
> src/gpu-compute/local_memory_pipeline.cc PRE-CREATION
> src/gpu-compute/misc.hh PRE-CREATION
> src/gpu-compute/ndrange.hh PRE-CREATION
> src/gpu-compute/of_scheduling_policy.hh PRE-CREATION
> src/gpu-compute/of_scheduling_policy.cc PRE-CREATION
> src/gpu-compute/pool_manager.hh PRE-CREATION
> src/gpu-compute/pool_manager.cc PRE-CREATION
> src/gpu-compute/qstruct.hh PRE-CREATION
> src/gpu-compute/rr_scheduling_policy.hh PRE-CREATION
> src/gpu-compute/rr_scheduling_policy.cc PRE-CREATION
> src/gpu-compute/schedule_stage.hh PRE-CREATION
> src/gpu-compute/schedule_stage.cc PRE-CREATION
> src/gpu-compute/scheduler.hh PRE-CREATION
> src/gpu-compute/scheduler.cc PRE-CREATION
> src/gpu-compute/scheduling_policy.hh PRE-CREATION
> src/gpu-compute/scoreboard_check_stage.hh PRE-CREATION
> src/gpu-compute/scoreboard_check_stage.cc PRE-CREATION
> src/gpu-compute/shader.hh PRE-CREATION
> src/gpu-compute/shader.cc PRE-CREATION
> src/gpu-compute/simple_pool_manager.hh PRE-CREATION
> src/gpu-compute/simple_pool_manager.cc PRE-CREATION
> src/gpu-compute/tlb_coalescer.hh PRE-CREATION
> src/gpu-compute/tlb_coalescer.cc PRE-CREATION
> src/gpu-compute/vector_register_file.hh PRE-CREATION
> src/gpu-compute/vector_register_file.cc PRE-CREATION
> src/gpu-compute/vector_register_state.hh PRE-CREATION
> src/gpu-compute/vector_register_state.cc PRE-CREATION
> src/gpu-compute/wavefront.hh PRE-CREATION
> src/gpu-compute/wavefront.cc PRE-CREATION
> src/mem/protocol/GPU_RfO-SQC.sm PRE-CREATION
> src/mem/protocol/GPU_RfO-TCC.sm PRE-CREATION
> src/mem/protocol/GPU_RfO-TCCdir.sm PRE-CREATION
> src/mem/protocol/GPU_RfO-TCP.sm PRE-CREATION
> src/mem/protocol/GPU_RfO.slicc PRE-CREATION
> src/mem/protocol/GPU_VIPER-SQC.sm PRE-CREATION
> src/mem/protocol/GPU_VIPER-TCC.sm PRE-CREATION
> src/mem/protocol/GPU_VIPER-TCP.sm PRE-CREATION
> src/mem/protocol/GPU_VIPER.slicc PRE-CREATION
> src/mem/protocol/GPU_VIPER_Baseline.slicc PRE-CREATION
> src/mem/protocol/GPU_VIPER_Region-TCC.sm PRE-CREATION
> src/mem/protocol/GPU_VIPER_Region.slicc PRE-CREATION
> src/mem/protocol/MOESI_AMD_Base-CorePair.sm PRE-CREATION
> src/mem/protocol/MOESI_AMD_Base-L3cache.sm PRE-CREATION
> src/mem/protocol/MOESI_AMD_Base-Region-CorePair.sm PRE-CREATION
> src/mem/protocol/MOESI_AMD_Base-Region-dir.sm PRE-CREATION
> src/mem/protocol/MOESI_AMD_Base-Region-msg.sm PRE-CREATION
> src/mem/protocol/MOESI_AMD_Base-RegionBuffer.sm PRE-CREATION
> src/mem/protocol/MOESI_AMD_Base-RegionDir.sm PRE-CREATION
> src/mem/protocol/MOESI_AMD_Base-dir.sm PRE-CREATION
> src/mem/protocol/MOESI_AMD_Base-msg.sm PRE-CREATION
> src/mem/protocol/MOESI_AMD_Base-probeFilter.sm PRE-CREATION
> src/mem/protocol/MOESI_AMD_Base.slicc PRE-CREATION
> src/mem/protocol/RubySlicc_ComponentMapping.sm
> d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/protocol/RubySlicc_Exports.sm
> d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/protocol/RubySlicc_Types.sm
> d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/protocol/SConsopts d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/SConscript d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/profiler/Profiler.cc d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/slicc_interface/AbstractCacheEntry.hh
> d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/slicc_interface/AbstractController.hh
> d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/slicc_interface/AbstractController.cc
> d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/slicc_interface/RubySlicc_ComponentMapping.hh
> d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/structures/CacheMemory.hh
> d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/structures/CacheMemory.cc
> d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/structures/RubyCache.py
> d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/system/GPUCoalescer.hh PRE-CREATION
> src/mem/ruby/system/GPUCoalescer.cc PRE-CREATION
> src/mem/ruby/system/GPUCoalescer.py PRE-CREATION
> src/mem/ruby/system/RubyPort.hh d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/system/RubyPort.cc d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/system/RubySystem.cc d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/system/SConscript d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/system/Sequencer.hh d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/system/Sequencer.cc d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/system/Sequencer.py d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/mem/ruby/system/VIPERCoalescer.hh PRE-CREATION
> src/mem/ruby/system/VIPERCoalescer.cc PRE-CREATION
> src/mem/ruby/system/VIPERCoalescer.py PRE-CREATION
> src/mem/ruby/system/WeightedLRUPolicy.hh PRE-CREATION
> src/mem/ruby/system/WeightedLRUPolicy.cc PRE-CREATION
> src/mem/ruby/system/WeightedLRUReplacementPolicy.py PRE-CREATION
> src/mem/slicc/symbols/StateMachine.py
> d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/gpu-compute/fetch_stage.cc PRE-CREATION
> src/gpu-compute/fetch_unit.hh PRE-CREATION
> src/gpu-compute/fetch_unit.cc PRE-CREATION
> src/gpu-compute/global_memory_pipeline.hh PRE-CREATION
> src/gpu-compute/global_memory_pipeline.cc PRE-CREATION
> src/gpu-compute/gpu_dyn_inst.hh PRE-CREATION
> src/gpu-compute/gpu_dyn_inst.cc PRE-CREATION
> src/gpu-compute/gpu_exec_context.hh PRE-CREATION
> src/gpu-compute/gpu_exec_context.cc PRE-CREATION
> src/gpu-compute/gpu_static_inst.hh PRE-CREATION
> src/gpu-compute/gpu_static_inst.cc PRE-CREATION
> src/gpu-compute/gpu_tlb.hh PRE-CREATION
> src/gpu-compute/gpu_tlb.cc PRE-CREATION
> src/gpu-compute/hsa_code.hh PRE-CREATION
> src/gpu-compute/hsa_kernel_info.hh PRE-CREATION
> src/gpu-compute/hsa_object.hh PRE-CREATION
> SConstruct d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> build_opts/X86_MOESI_AMD_Base PRE-CREATION
> build_opts/gpu_RfO PRE-CREATION
> build_opts/gpu_VIPER PRE-CREATION
> build_opts/gpu_VIPER_Baseline PRE-CREATION
> build_opts/gpu_VIPER_Region PRE-CREATION
> configs/common/GPUTLBConfig.py PRE-CREATION
> configs/common/GPUTLBOptions.py PRE-CREATION
> configs/example/apu_se.py PRE-CREATION
> configs/example/ruby_gpu_random_test.py PRE-CREATION
> configs/ruby/AMD_Base_Constructor.py PRE-CREATION
> configs/ruby/GPU_RfO.py PRE-CREATION
> configs/ruby/GPU_VIPER.py PRE-CREATION
> configs/ruby/GPU_VIPER_Baseline.py PRE-CREATION
> configs/ruby/GPU_VIPER_Region.py PRE-CREATION
> configs/ruby/MOESI_AMD_Base.py PRE-CREATION
> configs/ruby/MemCntrlConstructor.py PRE-CREATION
> src/SConscript d9a0136ab8cc4b3cf4821d064140b857e60db0dd
> src/gpu-compute/GPU.py PRE-CREATION
> src/gpu-compute/LdsState.py PRE-CREATION
> src/gpu-compute/SConscript PRE-CREATION
> src/gpu-compute/X86GPUTLB.py PRE-CREATION
> src/gpu-compute/arch/SConscript PRE-CREATION
> src/gpu-compute/arch/hsail/Brig.h PRE-CREATION
> src/gpu-compute/arch/hsail/Brig_new.hpp PRE-CREATION
> src/gpu-compute/arch/hsail/SConscript PRE-CREATION
> src/gpu-compute/arch/hsail/SConsopts PRE-CREATION
> src/gpu-compute/arch/hsail/decoder.hh PRE-CREATION
> src/gpu-compute/arch/hsail/gen.py PRE-CREATION
> src/gpu-compute/arch/hsail/generic_types.hh PRE-CREATION
> src/gpu-compute/arch/hsail/generic_types.cc PRE-CREATION
> src/gpu-compute/arch/hsail/insts/branch.hh PRE-CREATION
> src/gpu-compute/arch/hsail/insts/branch.cc PRE-CREATION
> src/gpu-compute/arch/hsail/insts/decl.hh PRE-CREATION
> src/gpu-compute/arch/hsail/insts/gpu_static_inst.hh PRE-CREATION
> src/gpu-compute/arch/hsail/insts/gpu_static_inst.cc PRE-CREATION
> src/gpu-compute/arch/hsail/insts/main.cc PRE-CREATION
> src/gpu-compute/arch/hsail/insts/mem.hh PRE-CREATION
> src/gpu-compute/arch/hsail/insts/mem.cc PRE-CREATION
> src/gpu-compute/arch/hsail/insts/mem_impl.hh PRE-CREATION
> src/gpu-compute/arch/hsail/insts/pseudo_inst.cc PRE-CREATION
> src/gpu-compute/arch/hsail/operand.hh PRE-CREATION
> src/gpu-compute/arch/hsail/operand.cc PRE-CREATION
> src/gpu-compute/arch/hsail/types.hh PRE-CREATION
> src/gpu-compute/brig_object.hh PRE-CREATION
> src/gpu-compute/brig_object.cc PRE-CREATION
> src/gpu-compute/cl_driver.hh PRE-CREATION
> src/gpu-compute/cl_driver.cc PRE-CREATION
> src/gpu-compute/cl_event.hh PRE-CREATION
> src/gpu-compute/code_enums.hh PRE-CREATION
> src/gpu-compute/compute_unit.hh PRE-CREATION
> src/gpu-compute/compute_unit.cc PRE-CREATION
> src/gpu-compute/condition_register_state.hh PRE-CREATION
> src/gpu-compute/condition_register_state.cc PRE-CREATION
> src/gpu-compute/dispatcher.hh PRE-CREATION
> src/gpu-compute/dispatcher.cc PRE-CREATION
> src/gpu-compute/exec_stage.hh PRE-CREATION
> src/gpu-compute/exec_stage.cc PRE-CREATION
> src/gpu-compute/fetch_stage.hh PRE-CREATION
>
> Diff: http://reviews.gem5.org/r/3189/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Tony Gutierrez
>
>
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
