> On Nov. 10, 2015, 7:51 p.m., Andreas Sandberg wrote: > > This is indeed very impressive! However, as it is, this patch is basically > > the definition of unreviewable. Could you split it into a handful of > > different patches for review purposes? ReviewBoard makes all kinds of > > disconcerting creaking noises when I try to browse the massive (almost > > 2MiB!) patch. Could you split it into a set of different patches, at least > > for review purposes? > > > > I'd suggest something along these lines: > > - Memory system stuff (Ruby) > > - GPU memory components (TLB/page table walkers etc.) > > - Configuration > > - Main build system (i.e., not the scons files in gpu/) > > - Pipeline > > - Object loader(s) > > - ISA > > > > Quickly eyeing the patch, I noticed two things: > > - The new GPU ISA constant lives in the same header as the CPU ISA. > > Please put it in a separate header. This will make things easier down the > > line if we ever decide to reduce the set of files we need to recompile for > > different ISAs. > > - Some classes in gpu/ live in the X86ISA name space. This seems wrong. > > Andreas Hansson wrote: > In addition to these concerns, there also seems to be a license header > format that does not appear anywhere else in the codebase. Is there a good > reason for this? > > Tony Gutierrez wrote: > That is the license required by our legal team. It's BSD-style, so it > shouldn't be an issue. > > Andreas Hansson wrote: > I just picked up on the fact that it's different, and was not sure why. > It should not be a problem, but if things can be uniform across the code base > that's probably easier for everyone. > > Brad Beckmann wrote: > There is nothing we can do about it. We are going to check it in using > the current license. By the way, ARM and HP broke the uniformity of the > licenses several years ago. There is really nothing new here. > > Andreas Hansson wrote: > That is fine. As I pointed out, I am merely observing (and enquiring > about) the fact that AMD has previously used a different license wording and > formatting. The ARM license header is consistent throughout the code base. > > Steve Reinhardt wrote: > We had previously received AMD legal approval to contribute bug fixes and > feature enhancements to M5/gem5 under the existing license. Since the GPU > model is a much more significant contribution of AMD IP, we had to go back to > legal for further approvals, which led to them insisting on the new "For use > for simulation and test purposes only" clause. How we ended up with > different formatting on the standard BSD part in the process is a mystery to > me though. > > Andreas Hansson wrote: > Thanks for the clarification. > > Andreas Hansson wrote: > What is the plan for the initial high-level comments from Andreas > Sandberg?
Here is our plan to address the two items noticed by Andreas Sandberg: - We will follow his suggestion and put the GPU ISA constant in a separate header. That should be easy enough to do. - I think the confusion here is that the gpu tlb. The gpu tlb is essentially a coalescing TLB structure that is specific to the host CPU ISA. Currently the only CPU ISA supported by the GPU TLB is x86. How about we move the GPU TLB to src/arch/x86? I think that will better follow the current design strategy and address that comment. - Brad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3189/#review7539 ----------------------------------------------------------- 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 gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
