> On Jan. 14, 2016, 6:26 p.m., Andreas Hansson wrote: > > configs/ruby/GPU_RfO.py, line 80 > > <http://reviews.gem5.org/r/3189/diff/3/?file=53131#file53131line80> > > > > there seems to be an awful lot of repetition of boilerplate code > > involved in these scripts, can the common parts not be broken out? > > Tony Gutierrez wrote: > They most likely can be, but I think such cleanup is better left for a > later patch where we can focus solely on code cleanup. > > Andreas Hansson wrote: > Again I must say think it is an odd way of approaching the review > process, but leave it up to you. If someone wants to use the patch now they > can download it and apply it. The normal review process is post -> iterate > based on feedback -> get to a happy spot -> commit. If the run scripts had > been separated from the actual compute-gpu functionality that process would > be far easier... > > Tony Gutierrez wrote: > We agree in principle, however given our current situation we think it's > easier to do post-fixup as opposed to have to maintain these patches, which > is a huge pain for us. We're trying to make some systematic changes > internally to avoid situations like the one we face at present. Going forward > our code will be in a more acceptable state for reviewer consumption. > > Brad Beckmann wrote: > Also I would add that these protocol scripts are designed in the same way > as all prior Ruby protocol config scripts. For instance, there is a lot of > duplicate code between the MESI_Three_Level.py and MESI_Two_Level.py files. > There is nothing out of the ordinary here. > > Andreas Hansson wrote: > I would be pretty sad if everyone argued like that. The only way to fix > the broken situation is for someone to actually do it properly. If you want > to do it in a separate patch, go for it. I'd still suggest to do get it right > from the start. > > Tony Gutierrez wrote: > Again, these problems with gem5's config system have existed for a long > time. We are not creating this problem, and it is unfair to ask us to fix the > problem before contributing our code.
I'm not asking you to fix what is there...just to not add fuel to the fire. Again, it is up to you, but sounds like we agree that the new scripts are not particularly well designed. > On Jan. 14, 2016, 6:26 p.m., Andreas Hansson wrote: > > SConstruct, line 1244 > > <http://reviews.gem5.org/r/3189/diff/3/?file=53120#file53120line1244> > > > > This makes it look like the two are not orthogonal, could you > > explain/comment? > > Tony Gutierrez wrote: > We introduce switching headers for the GPU that are identically named to > the CPU switching headers (in some cases). So they are not orthogonal in that > sense. The headers for the GPU ISAs are then generated in the > build/GPUISA/gpu-compute/arch, and scons didn't seem to play nicely when I > tried to have two separated ISAs in arch, one GPU ISA and one CPU ISA. So I > chose to separate them. > > Andreas Hansson wrote: > I am somewhat surprised. So the CPUs will be built for ISA build/CPUISA/? > What about the GPU? Is it like Ruby with build/CPUISA_GPUISA_RUBYPROTOCOL? > > Tony Gutierrez wrote: > No, the GPUISA I put in the middle there was just a generic place holder > for any ISA built, e.g., think of the build_opts files. So, e.g., in the case > of the GPU code, we have build_opts/gpu_RfO/. So the build tree, with respect > to the arch directories will look something like this: > > build/gpu_RfO/arch/ # the traditional arch dir > build/gpu_RfO/gpu-compute/arch # where GPU-specific arch code goes. > switching headers, etc. > > Andreas Hansson wrote: > So the GPUISA implies a CPUISA? Why not make them independent (which > seems more intuitive to me at least)? > > Tony Gutierrez wrote: > Because, if I understand what you're asking, they are not dependent. The > GPU is entirely dependent on there being a CPU ISA, at this point X86. > > Tony Gutierrez wrote: > *Not independent. > > Andreas Hansson wrote: > Would it not be sensible for the GPUISA name to include the implied > CPUISA in that case? > > I guess we would have a build/X86, and a build/X86_XYZ. > > Joel Hestness wrote: > I've thought about this some more, and I feel it's only necessary to > specify CPU ISA and GPU ISA (without any associated CPU ISA). While it's true > that some GPUs will be dependent on also executing particular CPU ISAs, it's > possible that someone could circle back and adjust the devices/etc. required > to run the GPU under another CPU ISA. For example, gem5-gpu has NVIDIA > CUDA/PTX/SASS GPUs from GPGPU-Sim, and we've set up our infrastructure to > allow either x86 or ARM32 as the CPU ISA. Our gem5 build_opts are named > X86_<rubyprotocol>_GPU and ARM_<rubyprotocol>_GPU, respectively. A minimal > amount of code is dependent on the "#define TARGET_ISA" for things like > address translation and data packing, but the vast majority is > interchangeable across CPU ISAs. If we were to add another GPU, it would make > sense to just change "_GPU" in the build_opts names to the GPU ISA. > > Overall, while CPU and GPU ISA may currently be dependent, they are > likely to become more independent over time, so I feel that including the > separate ISAs without denoting dependence should be sufficient. What would it look like in this instance? Also, how many additional builds are we ending up with? I guess the cross-product of Ruby protocols and GPU ISAs (NULL and HSAIL)? - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3189/#review7897 ----------------------------------------------------------- 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
