> On Nov. 13, 2015, 11:03 p.m., Joel Hestness wrote: > > src/mem/protocol/RubySlicc_Exports.sm, line 243 > > <http://reviews.gem5.org/r/3189/diff/1/?file=51125#file51125line243> > > > > Please describe these more clearly. What is a CorePair? What do "TCP", > > "TCC", "SQC" stand for? Etc. > > > > *Note: By adding machine types to this enumeration, you are declaring > > them as globally visible across any protocol that cares to use them. If we > > allow ambiguous or inaccurate descriptions here, various protocols may use > > them in different ways that make the intended naming unclear. If too > > ambiguous, users will resort to adding more names, bloating this > > enumeration, and causing this file to always have merge conflicts.
Updated. I don't think there is more we can do here. Please note that almost all the descriptions in this file are very minimal. > On Nov. 13, 2015, 11:03 p.m., Joel Hestness wrote: > > src/mem/protocol/RubySlicc_Types.sm, line 195 > > <http://reviews.gem5.org/r/3189/diff/1/?file=51126#file51126line195> > > > > Does declaring this here cause conflicts with the declarations in > > MOESI_CMP_directory-L2cache.sm and MOESI_CMP_token-L2cache.sm? Even if > > SLICC is somehow able to manage multiple definitions, it would be good to > > avoid the redundant declarations. Maybe we could move this change out of > > this patch to a separate patch that also removes the old declarations? Thank you for catching this. This change is unnecessary and has now been removed. > On Nov. 13, 2015, 11:03 p.m., Joel Hestness wrote: > > src/mem/ruby/slicc_interface/RubySlicc_ComponentMapping.hh, line 47 > > <http://reviews.gem5.org/r/3189/diff/1/?file=51133#file51133line47> > > > > This function is the same as map_Address_to_DirectoryNode(). Why > > replicate it? To make the code more readable and to allow other patches to update it for more optimized mapping policies. > On Nov. 13, 2015, 11:03 p.m., Joel Hestness wrote: > > src/mem/ruby/structures/CacheMemory.hh, line 110 > > <http://reviews.gem5.org/r/3189/diff/1/?file=51135#file51135line110> > > > > I don't see where this function is used in any of these patches. Can > > you please verify that it is necessary? It is used by internal code. > On Nov. 13, 2015, 11:03 p.m., Joel Hestness wrote: > > src/mem/ruby/structures/CacheMemory.hh, line 150 > > <http://reviews.gem5.org/r/3189/diff/1/?file=51135#file51135line150> > > > > I don't see where this function is used in any of these patches. Can > > you please verify that it is necessary? It is used by internal code. > On Nov. 13, 2015, 11:03 p.m., Joel Hestness wrote: > > src/mem/ruby/system/GPUCoalescer.hh, line 80 > > <http://reviews.gem5.org/r/3189/diff/1/?file=51138#file51138line80> > > > > This guy looks an awful lot like a Sequencer, so there is a lot of > > replicated code between here and the Sequencer. It seems like better > > abstraction needs to be used. Could GPUCoalescer descend from Sequencer > > instead, and just overload/add functionality that is different? > > > > I've already noted ramifications this has in the profiler patch (3192). > > I'm not sure if it's appropriate to address this before checking in the > > GPU, but if not, it should certainly be addressed shortly after. I'd prefer > > a plan of attack. I believe we've already discussed your concerns in a prior conversation. I'd prefer not to rehash that conversation again. There is a lot of complexity here and many subtle issues have huge performance implications. Let's not get hung up on this now. Eventually, we need to overall all the RubyPort implementations to support two buffers. One for requests recieved, but not yet issued to the mandatory queue and the other for requests issued to the mandatory queue. I think that will simply/improve the interface and it will be clearer what code can be shared between different implementations of the RubyPort. > On Nov. 13, 2015, 11:03 p.m., Joel Hestness wrote: > > src/mem/ruby/structures/CacheMemory.hh, line 187 > > <http://reviews.gem5.org/r/3189/diff/1/?file=51135#file51135line187> > > > > Couple things: > > 1) I see that initializing variables like this in the class declaration > > is new C++11 functionality. This kind of initialization is inconsistent > > with all other class member declarations in gem5, though, and this > > particular initialization doesn't appear to be necessary (see below). > > > > 2) The m_block_size variable only appears to be used in > > CacheMemory::init(). Do we need to add it, or can it just be a local > > variable in init()? Thank you for pointing this out. I did not write this code, but I'll make sure that this is necessary. Perhaps it was necessary to fix a problem flagged by the compiler??? Also I'll figure out that Brandon/Nilay's multi-instance ruby code fixes this problem. > On Nov. 13, 2015, 11:03 p.m., Joel Hestness wrote: > > src/mem/ruby/structures/CacheMemory.cc, line 76 > > <http://reviews.gem5.org/r/3189/diff/1/?file=51136#file51136line76> > > > > This looks like it could result in bugs. m_block_size is initialized to > > -1 in CacheMemory.hh, and possibly to sizes other than > > RubySystem::getBlockSizeBytes() in the constructor. Seems like it could > > either be incorrectly set and not 0 at the time init() is called (e.g. 2), > > or the initialization to -1 is unnecessary. > > > > 1) Why not require that it be set when a CacheMemory is instantiated in > > Python? > > 2) or, why not remove the initialization in the class declaration? I'll get back to you. > On Nov. 13, 2015, 11:03 p.m., Joel Hestness wrote: > > src/mem/ruby/system/Sequencer.py, line 77 > > <http://reviews.gem5.org/r/3189/diff/1/?file=51146#file51146line77> > > > > Why is this default initialized to 16? Thank you for pointing this out. We will remove the default setting of 16. - Brad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3189/#review7453 ----------------------------------------------------------- On Dec. 23, 2015, 5:11 p.m., Tony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3189/ > ----------------------------------------------------------- > > (Updated Dec. 23, 2015, 5:11 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11281:ed03615555f1 > --------------------------- > gpu: AMD's baseline GPU model > > This patch includes the entire baseline GPU model from AMD. The patch needs > to be checked in as one changeset, but let us know if you would prefer it to > be split for reviewing purposes. > > > Diffs > ----- > > src/gpu/global_memory_pipeline.hh PRE-CREATION > src/gpu/global_memory_pipeline.cc PRE-CREATION > src/gpu/gpu_dyn_inst.hh PRE-CREATION > src/gpu/gpu_dyn_inst.cc PRE-CREATION > src/gpu/gpu_exec_context.hh PRE-CREATION > src/gpu/gpu_exec_context.cc PRE-CREATION > src/gpu/gpu_static_inst.hh PRE-CREATION > src/gpu/gpu_static_inst.cc PRE-CREATION > src/gpu/gpu_tlb.hh PRE-CREATION > src/gpu/gpu_tlb.cc PRE-CREATION > src/gpu/hsa_code.hh PRE-CREATION > src/gpu/hsa_kernel_info.hh PRE-CREATION > src/gpu/hsa_object.hh PRE-CREATION > src/gpu/hsa_object.cc PRE-CREATION > src/gpu/code_enums.hh PRE-CREATION > src/gpu/compute_unit.hh PRE-CREATION > src/gpu/compute_unit.cc PRE-CREATION > src/gpu/condition_register_state.hh PRE-CREATION > src/gpu/condition_register_state.cc PRE-CREATION > src/gpu/dispatcher.hh PRE-CREATION > src/gpu/dispatcher.cc PRE-CREATION > src/gpu/exec_stage.hh PRE-CREATION > src/gpu/exec_stage.cc PRE-CREATION > src/gpu/fetch_stage.hh PRE-CREATION > src/gpu/fetch_stage.cc PRE-CREATION > src/gpu/fetch_unit.hh PRE-CREATION > src/gpu/fetch_unit.cc PRE-CREATION > src/mem/protocol/MOESI_AMD_Base.slicc 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-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/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/gpu/hsail_code.hh PRE-CREATION > src/gpu/hsail_code.cc PRE-CREATION > src/gpu/kernel_cfg.hh PRE-CREATION > src/gpu/kernel_cfg.cc PRE-CREATION > src/gpu/lds_state.hh PRE-CREATION > src/gpu/lds_state.cc PRE-CREATION > src/gpu/local_memory_pipeline.hh PRE-CREATION > src/gpu/local_memory_pipeline.cc PRE-CREATION > src/gpu/misc.hh PRE-CREATION > src/gpu/ndrange.hh PRE-CREATION > src/gpu/of_scheduling_policy.hh PRE-CREATION > src/gpu/of_scheduling_policy.cc PRE-CREATION > src/gpu/pool_manager.hh PRE-CREATION > src/gpu/pool_manager.cc PRE-CREATION > src/gpu/qstruct.hh PRE-CREATION > src/gpu/rr_scheduling_policy.hh PRE-CREATION > src/gpu/rr_scheduling_policy.cc PRE-CREATION > src/gpu/schedule_stage.hh PRE-CREATION > src/gpu/schedule_stage.cc PRE-CREATION > src/gpu/scheduler.hh PRE-CREATION > src/gpu/scheduler.cc PRE-CREATION > src/gpu/scheduling_policy.hh PRE-CREATION > src/gpu/scoreboard_check_stage.hh PRE-CREATION > src/gpu/scoreboard_check_stage.cc PRE-CREATION > src/gpu/shader.hh PRE-CREATION > src/gpu/shader.cc PRE-CREATION > src/gpu/simple_pool_manager.hh PRE-CREATION > src/gpu/simple_pool_manager.cc PRE-CREATION > src/gpu/tlb_coalescer.hh PRE-CREATION > src/gpu/tlb_coalescer.cc PRE-CREATION > src/gpu/vector_register_file.hh PRE-CREATION > src/gpu/vector_register_file.cc PRE-CREATION > src/gpu/vector_register_state.hh PRE-CREATION > src/gpu/vector_register_state.cc PRE-CREATION > src/gpu/wavefront.hh PRE-CREATION > src/gpu/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/gpu/arch/hsail/operand.cc PRE-CREATION > src/gpu/arch/hsail/types.hh PRE-CREATION > src/gpu/brig_object.hh PRE-CREATION > src/gpu/brig_object.cc PRE-CREATION > src/gpu/cl_driver.hh PRE-CREATION > src/gpu/cl_driver.cc PRE-CREATION > src/gpu/cl_event.hh PRE-CREATION > src/gpu/arch/hsail/insts/gpu_static_inst.cc PRE-CREATION > src/gpu/arch/hsail/insts/main.cc PRE-CREATION > src/gpu/arch/hsail/insts/mem.hh PRE-CREATION > src/gpu/arch/hsail/insts/mem.cc PRE-CREATION > src/gpu/arch/hsail/insts/mem_impl.hh PRE-CREATION > src/gpu/arch/hsail/insts/pseudo_inst.cc PRE-CREATION > src/gpu/arch/hsail/operand.hh PRE-CREATION > src/gpu/arch/hsail/generic_types.cc PRE-CREATION > src/gpu/arch/hsail/insts/branch.hh PRE-CREATION > src/gpu/arch/hsail/insts/branch.cc PRE-CREATION > src/gpu/arch/hsail/insts/decl.hh PRE-CREATION > src/gpu/arch/hsail/insts/gpu_static_inst.hh 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/GPU.py PRE-CREATION > src/gpu/LdsState.py PRE-CREATION > src/gpu/SConscript PRE-CREATION > src/gpu/X86GPUTLB.py PRE-CREATION > src/gpu/arch/SConscript PRE-CREATION > src/gpu/arch/hsail/Brig.h PRE-CREATION > src/gpu/arch/hsail/Brig_new.hpp PRE-CREATION > src/gpu/arch/hsail/SConscript PRE-CREATION > src/gpu/arch/hsail/SConsopts PRE-CREATION > src/gpu/arch/hsail/decoder.hh PRE-CREATION > src/gpu/arch/hsail/gen.py PRE-CREATION > src/gpu/arch/hsail/generic_types.hh 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 > 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 > src/mem/ruby/structures/RubyCache.py > 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/protocol/RubySlicc_Types.sm > d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/protocol/SConsopts d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/protocol/RubySlicc_Exports.sm > d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/protocol/RubySlicc_ComponentMapping.sm > d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/ruby/system/GPUCoalescer.hh PRE-CREATION > src/mem/ruby/system/GPUCoalescer.cc PRE-CREATION > src/mem/slicc/symbols/StateMachine.py > d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/ruby/system/WeightedLRUReplacementPolicy.py PRE-CREATION > src/mem/ruby/system/WeightedLRUPolicy.cc PRE-CREATION > src/mem/ruby/system/WeightedLRUPolicy.hh PRE-CREATION > src/mem/ruby/system/VIPERCoalescer.py PRE-CREATION > src/mem/ruby/system/VIPERCoalescer.cc PRE-CREATION > src/mem/ruby/system/VIPERCoalescer.hh PRE-CREATION > src/mem/ruby/system/Sequencer.cc d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/ruby/system/Sequencer.py d9a0136ab8cc4b3cf4821d064140b857e60db0dd > src/mem/ruby/system/Sequencer.hh d9a0136ab8cc4b3cf4821d064140b857e60db0dd > 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/GPUCoalescer.py 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
