> On May 13, 2016, 3:40 a.m., Steve Reinhardt wrote: > > Nice! It's great to see someone unafraid to take on big cleanup tasks like > > this. Several little things below that could be tweaked, but on the whole > > it's a very positive change! Thanks!
Thanks for your review! I've fixed most of the things you pointed out. See the comments for the details. > On May 13, 2016, 3:40 a.m., Steve Reinhardt wrote: > > src/arch/arm/insts/branch64.cc, line 98 > > <http://reviews.gem5.org/r/3457/diff/1/?file=55164#file55164line98> > > > > would be more compact (and more consistent with other functions, not to > > mention potentially more efficient) to just define a printIntReg() function > > rather than having to explicitly add IntRegClass here, then test the value > > later printIntReg() / printFloatReg() / printMiscReg() / printCCReg() functions have been added. The code has been modified to use them. > On May 13, 2016, 3:40 a.m., Steve Reinhardt wrote: > > src/arch/arm/insts/static_inst.cc, line 297 > > <http://reviews.gem5.org/r/3457/diff/1/?file=55173#file55173line297> > > > > this is the switch you could get rid of if you switched to separate > > printIntReg() / printFloatReg() / printMiscReg() / printCCReg() functions Done. > On May 13, 2016, 3:40 a.m., Steve Reinhardt wrote: > > src/arch/isa_parser.py, line 536 > > <http://reviews.gem5.org/r/3457/diff/1/?file=55177#file55177line536> > > > > why the extra parens? Fixed. > On May 13, 2016, 3:40 a.m., Steve Reinhardt wrote: > > src/arch/isa_parser.py, line 609 > > <http://reviews.gem5.org/r/3457/diff/1/?file=55177#file55177line609> > > > > Since these strings get used over and over, I'd say we should create > > some module-level string variables, e.g.: > > > > src_reg_constructor = '\n\t_srcRegIdx[_numSrcRegs++] = RegId(%s, %s);' > > > > In fact, if we create a couple of class-level attributes for reg_class > > and reg_class_counter (e.g., '_numIntDestRegs'), then we can probably > > define a single common makeConstructor() method for the Operand class src_reg_constructor and dst_reg_constructor have been added and are used where it makes sense. reg_class is now a class-level attribute. Complete refactoring of the makeConstructor() is more difficult because not all operands do exactly the same things for it. So it will to be addressed in an other patch. > On May 13, 2016, 3:40 a.m., Steve Reinhardt wrote: > > src/arch/mips/isa/decoder.isa, line 397 > > <http://reviews.gem5.org/r/3457/diff/1/?file=55179#file55179line397> > > > > might be worth defining a macro or helper function to clean this up... > > then again maybe not. I'm not really sure on how to define macro in .isa file. And it would be a bit outside of the scope of this change. > On May 13, 2016, 3:40 a.m., Steve Reinhardt wrote: > > src/arch/x86/insts/static_inst.hh, line 59 > > <http://reviews.gem5.org/r/3457/diff/1/?file=55198#file55198line59> > > > > would be nice to get rid of this eventually... please add a TODO > > comment explaining why it's too daunting to do here :) TODO added. To be fair, I have tried to not define this function in the first place. But my understand of the ISA definition in X86 is too limited for that. > On May 13, 2016, 3:40 a.m., Steve Reinhardt wrote: > > src/cpu/o3/cpu.cc, line 789 > > <http://reviews.gem5.org/r/3457/diff/1/?file=55212#file55212line789> > > > > could use the struct directly, I think, e.g.: > > > > for (RegId reg_id(IntRegClass, 0); reg_id.regIdx < > > TheISA::NumIntRegs; > > reg_id.regIdx++) { > > [...] > > } > > > > not a huge deal here, but should avoid breaking the assignment across > > multiple lines in removeThread() below Done. > On May 13, 2016, 3:40 a.m., Steve Reinhardt wrote: > > src/cpu/reg_class.hh, line 76 > > <http://reviews.gem5.org/r/3457/diff/1/?file=55221#file55221line76> > > > > use initializer list > > > > : regClass(reg_class), regIdx(reg_idx) Done. > On May 13, 2016, 3:40 a.m., Steve Reinhardt wrote: > > src/cpu/reg_class.hh, line 80 > > <http://reviews.gem5.org/r/3457/diff/1/?file=55221#file55221line80> > > > > blank lines between methods (here & below) Done. > On May 13, 2016, 3:40 a.m., Steve Reinhardt wrote: > > src/cpu/reg_class.hh, line 84 > > <http://reviews.gem5.org/r/3457/diff/1/?file=55221#file55221line84> > > > > I find > > return !(*this==that); > > more intuitive (subjective though) Both are ok for me so I've picked yours. > On May 13, 2016, 3:40 a.m., Steve Reinhardt wrote: > > src/cpu/reg_class.hh, line 102 > > <http://reviews.gem5.org/r/3457/diff/1/?file=55221#file55221line102> > > > > I would prefer > > > > return (regIdx == TheISA::ZeroReg && > > (regClass == IntRegClass || > > (THE_ISA == ALPHA_ISA && regClass == FloatRegClass))); Fixed. - Nathanael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3457/#review8301 ----------------------------------------------------------- On May 23, 2016, 4:47 p.m., Andreas Sandberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3457/ > ----------------------------------------------------------- > > (Updated May 23, 2016, 4:47 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11464:d0b3c18e9e75 > --------------------------- > arch, cpu: Architectural Register structural indexing > > Replace the unified register mapping by a structure associating a > class and an index. It is now much easier to know which class of > register the index is refering to. Also, when adding a new class > there is no need to modify existing ones. > > Change-Id: I55b3ac80763702aa2cd3ed2cbff0a75ef7620373 > Reviewed-by: Andreas Sandberg <andreas.sandb...@arm.com> > > > Diffs > ----- > > src/arch/alpha/isa/branch.isa d9e32a851e2e > src/arch/alpha/isa/fp.isa d9e32a851e2e > src/arch/alpha/isa/main.isa d9e32a851e2e > src/arch/alpha/registers.hh d9e32a851e2e > src/arch/arm/insts/branch64.cc d9e32a851e2e > src/arch/arm/insts/data64.cc d9e32a851e2e > src/arch/arm/insts/macromem.cc d9e32a851e2e > src/arch/arm/insts/mem.hh d9e32a851e2e > src/arch/arm/insts/mem.cc d9e32a851e2e > src/arch/arm/insts/mem64.cc d9e32a851e2e > src/arch/arm/insts/misc.cc d9e32a851e2e > src/arch/arm/insts/misc64.cc d9e32a851e2e > src/arch/arm/insts/static_inst.hh d9e32a851e2e > src/arch/arm/insts/static_inst.cc d9e32a851e2e > src/arch/arm/insts/vfp.cc d9e32a851e2e > src/arch/arm/registers.hh d9e32a851e2e > src/arch/generic/types.hh d9e32a851e2e > src/arch/isa_parser.py d9e32a851e2e > src/arch/mips/isa/base.isa d9e32a851e2e > src/arch/mips/isa/decoder.isa d9e32a851e2e > src/arch/mips/isa/formats/int.isa d9e32a851e2e > src/arch/mips/isa/formats/mt.isa d9e32a851e2e > src/arch/mips/mt.hh d9e32a851e2e > src/arch/mips/registers.hh d9e32a851e2e > src/arch/null/registers.hh d9e32a851e2e > src/arch/power/insts/branch.cc d9e32a851e2e > src/arch/power/insts/static_inst.hh d9e32a851e2e > src/arch/power/insts/static_inst.cc d9e32a851e2e > src/arch/power/registers.hh d9e32a851e2e > src/arch/sparc/isa/base.isa d9e32a851e2e > src/arch/sparc/isa/formats/integerop.isa d9e32a851e2e > src/arch/sparc/isa/formats/mem/util.isa d9e32a851e2e > src/arch/sparc/isa/formats/priv.isa d9e32a851e2e > src/arch/sparc/registers.hh d9e32a851e2e > src/arch/x86/insts/microfpop.hh d9e32a851e2e > src/arch/x86/insts/microldstop.hh d9e32a851e2e > src/arch/x86/insts/micromediaop.hh d9e32a851e2e > src/arch/x86/insts/microregop.hh d9e32a851e2e > src/arch/x86/insts/static_inst.hh d9e32a851e2e > src/arch/x86/insts/static_inst.cc d9e32a851e2e > src/arch/x86/isa/microops/limmop.isa d9e32a851e2e > src/arch/x86/isa/specialize.isa d9e32a851e2e > src/arch/x86/registers.hh d9e32a851e2e > src/cpu/base_dyn_inst.hh d9e32a851e2e > src/cpu/checker/cpu.hh d9e32a851e2e > src/cpu/checker/cpu_impl.hh d9e32a851e2e > src/cpu/exec_context.hh d9e32a851e2e > src/cpu/minor/dyn_inst.hh d9e32a851e2e > src/cpu/minor/dyn_inst.cc d9e32a851e2e > src/cpu/minor/exec_context.hh d9e32a851e2e > src/cpu/minor/scoreboard.hh d9e32a851e2e > src/cpu/minor/scoreboard.cc d9e32a851e2e > src/cpu/o3/cpu.cc d9e32a851e2e > src/cpu/o3/dyn_inst.hh d9e32a851e2e > src/cpu/o3/dyn_inst_impl.hh d9e32a851e2e > src/cpu/o3/probe/elastic_trace.cc d9e32a851e2e > src/cpu/o3/rename.hh d9e32a851e2e > src/cpu/o3/rename_impl.hh d9e32a851e2e > src/cpu/o3/rename_map.hh d9e32a851e2e > src/cpu/o3/rename_map.cc d9e32a851e2e > src/cpu/o3/rob.hh d9e32a851e2e > src/cpu/reg_class.hh d9e32a851e2e > src/cpu/reg_class.cc d9e32a851e2e > src/cpu/simple/exec_context.hh d9e32a851e2e > src/cpu/static_inst.hh d9e32a851e2e > src/cpu/thread_context.hh d9e32a851e2e > src/cpu/timing_expr.cc d9e32a851e2e > > Diff: http://reviews.gem5.org/r/3457/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Sandberg > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
