----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3189/#review7453 -----------------------------------------------------------
Looking forward to seeing the GPU in action! I'm mostly fine with new files being added as-is, assuming they pass the style checker. Below are notes mostly regarding the changes/additions in existing gem5 code. src/gpu/gpu_mem_packet.cc (line 214) <http://reviews.gem5.org/r/3189/#comment6297> Also noted in the GPU atomics patch: Why not just return a pointer to the type-specific functions rather than dynamically instantiating wrappers around them for each atomic Request? src/mem/protocol/RubySlicc_Exports.sm (line 243) <http://reviews.gem5.org/r/3189/#comment6496> 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. src/mem/protocol/RubySlicc_Types.sm (line 195) <http://reviews.gem5.org/r/3189/#comment6511> 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? src/mem/ruby/slicc_interface/RubySlicc_ComponentMapping.hh (line 47) <http://reviews.gem5.org/r/3189/#comment6497> This function is the same as map_Address_to_DirectoryNode(). Why replicate it? src/mem/ruby/slicc_interface/RubySlicc_ComponentMapping.hh (line 128) <http://reviews.gem5.org/r/3189/#comment6498> This function is functionally identical to createMachineID. Why add a separate function? src/mem/ruby/structures/CacheMemory.hh (line 110) <http://reviews.gem5.org/r/3189/#comment6512> I don't see where this function is used in any of these patches. Can you please verify that it is necessary? src/mem/ruby/structures/CacheMemory.hh (line 150) <http://reviews.gem5.org/r/3189/#comment6513> I don't see where this function is used in any of these patches. Can you please verify that it is necessary? src/mem/ruby/structures/CacheMemory.hh (line 187) <http://reviews.gem5.org/r/3189/#comment6493> 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()? src/mem/ruby/structures/CacheMemory.cc (line 76) <http://reviews.gem5.org/r/3189/#comment6492> 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? src/mem/ruby/system/GPUCoalescer.hh (line 80) <http://reviews.gem5.org/r/3189/#comment6473> 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. src/mem/ruby/system/RubyPort.hh (line 170) <http://reviews.gem5.org/r/3189/#comment6494> Just a note: This is basically the accessor function I am referring to when I suggest that the SLICC generated getCPUSequencer and getGPUCoalescer functions should be merged to return a type from which both Sequencer and GPUCoalescer inherit. src/mem/ruby/system/Sequencer.py (line 77) <http://reviews.gem5.org/r/3189/#comment6495> Why is this default initialized to 16? - Joel Hestness On Oct. 30, 2015, 9:52 p.m., Tony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3189/ > ----------------------------------------------------------- > > (Updated Oct. 30, 2015, 9:52 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11195:e9e245ab10f9 > --------------------------- > 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/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 > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/slicc_interface/AbstractController.cc > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/slicc_interface/RubySlicc_ComponentMapping.hh > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/slicc_interface/RubySlicc_Util.hh > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/structures/CacheMemory.hh > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/structures/CacheMemory.cc > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/structures/RubyCache.py > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/system/GPUCoalescer.hh PRE-CREATION > src/mem/ruby/system/GPUCoalescer.cc PRE-CREATION > src/mem/ruby/system/RubyPort.hh 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/system/RubyPort.cc 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/system/RubySystem.cc 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/system/SConscript 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/system/Sequencer.hh 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/system/Sequencer.cc 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/system/Sequencer.py 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/system/VIPERCoalescer.hh PRE-CREATION > src/mem/ruby/system/VIPERCoalescer.cc 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/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 > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/protocol/RubySlicc_Exports.sm > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/protocol/RubySlicc_Types.sm > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/protocol/SConsopts 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/SConscript 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/profiler/Profiler.cc 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/slicc_interface/AbstractCacheEntry.hh > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > src/mem/ruby/slicc_interface/AbstractController.hh > 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > 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/gpu_mem_packet.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/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/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/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/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/gpu/global_memory_pipeline.hh PRE-CREATION > src/gpu/global_memory_pipeline.cc PRE-CREATION > src/gpu/gpu_mem_packet.hh PRE-CREATION > SConstruct 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > 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/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 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > 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/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 > 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 > > 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
