----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3457/#review8301 -----------------------------------------------------------
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! src/arch/arm/insts/branch64.cc (line 98) <http://reviews.gem5.org/r/3457/#comment7118> 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 src/arch/arm/insts/static_inst.cc (line 297) <http://reviews.gem5.org/r/3457/#comment7119> this is the switch you could get rid of if you switched to separate printIntReg() / printFloatReg() / printMiscReg() / printCCReg() functions src/arch/isa_parser.py (line 536) <http://reviews.gem5.org/r/3457/#comment7116> why the extra parens? src/arch/isa_parser.py (line 609) <http://reviews.gem5.org/r/3457/#comment7117> 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/arch/mips/isa/decoder.isa (line 397) <http://reviews.gem5.org/r/3457/#comment7126> might be worth defining a macro or helper function to clean this up... then again maybe not. src/arch/x86/insts/static_inst.hh (line 59) <http://reviews.gem5.org/r/3457/#comment7127> would be nice to get rid of this eventually... please add a TODO comment explaining why it's too daunting to do here :) src/cpu/o3/cpu.cc (line 787) <http://reviews.gem5.org/r/3457/#comment7124> 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 src/cpu/o3/cpu.cc (line 839) <http://reviews.gem5.org/r/3457/#comment7125> if for some reason you do end up needing to break an assignment like this across two lines, I think it's cleaner to break it at the '=' src/cpu/reg_class.hh (line 76) <http://reviews.gem5.org/r/3457/#comment7120> use initializer list : regClass(reg_class), regIdx(reg_idx) src/cpu/reg_class.hh (line 80) <http://reviews.gem5.org/r/3457/#comment7122> blank lines between methods (here & below) src/cpu/reg_class.hh (line 84) <http://reviews.gem5.org/r/3457/#comment7121> I find return !(*this==that); more intuitive (subjective though) src/cpu/reg_class.hh (line 92) <http://reviews.gem5.org/r/3457/#comment7123> I would prefer return (regIdx == TheISA::ZeroReg && (regClass == IntRegClass || (THE_ISA == ALPHA_ISA && regClass == FloatRegClass))); - Steve Reinhardt On April 28, 2016, 4:56 a.m., Andreas Sandberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3457/ > ----------------------------------------------------------- > > (Updated April 28, 2016, 4:56 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11464:810b3115a5d6 > --------------------------- > 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 > Signed-off-by: Andreas Sandberg <andreas.sandb...@arm.com> > > > Diffs > ----- > > 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 > 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/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/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/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/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 > > 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
