----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/20/#review15 -----------------------------------------------------------
Overall I'm kinda disappointed that most of the decode ended up in C++ and not in the ISA description language (I know the latter would have required some extensions)... now that it's in place and (presumably) working though, I guess there's no compelling reason to go back and do it that way. src/arch/arm/faults.hh <http://reviews.m5sim.org/r/20/#comment57> Seems redundant to have 'Arm' in the name of a class that's inside the 'ArmISA' namespace... I can see where there might be some confusion if there's a base Fault that's outside the namespace as well, but maybe that's a sign that there's a larger renaming that should be done. Also, Fault:FaultBase seems more consistent with other naming than FaultVals:Fault; I'm curious what motivated the change. src/arch/arm/insts/branch.cc <http://reviews.m5sim.org/r/20/#comment66> Is this file really empty now? If so, shouldn't we just get rid of it? src/arch/arm/insts/macromem.hh <http://reviews.m5sim.org/r/20/#comment67> Be nice not to have deleted the comments describing these classes... or at least write new ones. src/arch/arm/insts/mem.hh <http://reviews.m5sim.org/r/20/#comment68> seems like this should go in a .cc file src/arch/arm/insts/mem.hh <http://reviews.m5sim.org/r/20/#comment69> this should go in a .cc file too src/arch/arm/insts/static_inst.hh <http://reviews.m5sim.org/r/20/#comment70> don't you want these (incl. many more below) declared inline? src/arch/arm/insts/vfp.hh <http://reviews.m5sim.org/r/20/#comment71> Some comments would be nice... I assume vfp is "vector floating point", but it would be good to say that explicitly. src/arch/arm/insts/vfp.hh <http://reviews.m5sim.org/r/20/#comment72> does this need to be inline? seems like a lot of code (and the ones below are worse). src/arch/arm/insts/vfp.hh <http://reviews.m5sim.org/r/20/#comment77> What's the purpose of these __asm__ statements? src/arch/arm/insts/vfp.hh <http://reviews.m5sim.org/r/20/#comment73> zoinks, a 1200-line header? src/arch/arm/interrupts.hh <http://reviews.m5sim.org/r/20/#comment58> I guess this is subjective, but effectively coding a boolean expression as control flow seems weird to me unless it's *really* complicated otherwise. So I'd write this as: return ((interrupts[INT_IRQ] && !cpsr.i) || (interrupts[INT_FIQ] && !cpsr.f) || (interrupts[INT_ABT] && !cpsr.a) || interrupts[INT_RST]); src/arch/arm/isa.hh <http://reviews.m5sim.org/r/20/#comment60> This seems like a really big and rarely used function to be defined in the .hh file. src/arch/arm/isa.hh <http://reviews.m5sim.org/r/20/#comment59> I don't get this comment... it sounds like you're explaining why cpacr isn't 0 even though it should be, but then you set it to 0 anyway. src/arch/arm/isa/bitfields.isa <http://reviews.m5sim.org/r/20/#comment74> I assume these are all instances of your C++ bitfield class? Maybe we should add some first-class support in the ISA parser to get rid of this redundant-looking header. What are the advantages of doing it this way vs. the old way, anyway? src/arch/arm/isa/decoder/decoder.isa <http://reviews.m5sim.org/r/20/#comment76> It would be clearer to do: decode THUMB default Unknown::unknown() { 0: ##include "arm.isa" 1: ##include "thumb.isa" } then get rid of the context-free 0:/1: in the included files... (add line breaks if necessary if we require the '##' to be at the beginning of a line) src/arch/arm/isa/formats/data.isa <http://reviews.m5sim.org/r/20/#comment78> Is there a reason we can't use predefined bitfields here (and a ton of other places)? src/arch/arm/pagetable.hh <http://reviews.m5sim.org/r/20/#comment61> 'inline' is redundant here and on next method src/arch/arm/predecoder.hh <http://reviews.m5sim.org/r/20/#comment62> Can we move all this code to a .cc file? src/arch/arm/table_walker.hh <http://reviews.m5sim.org/r/20/#comment63> Should have a blank line between these function defs src/arch/arm/table_walker.hh <http://reviews.m5sim.org/r/20/#comment64> What is N? Unless this is obvious to anyone who knows anything about ARM page tables (a set that doesn't include me) a longer name would be nice. In any case, a comment is called for. src/arch/arm/utility.hh <http://reviews.m5sim.org/r/20/#comment65> What's the point of 'static' here (and on the previous function)? Also, this seems like a more generic "print binary" function... can we just add "%b" to cprintf? - Steve On 2010-04-29 15:33:58, Ali Saidi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/20/ > ----------------------------------------------------------- > > (Updated 2010-04-29 15:33:58) > > > Review request for Default. > > > Summary > ------- > > Initial set of patches to improve the M5 support of the ARM ISA. Bundled into > one large change for review. This change implements the majority of thumb, > thumb2, and arm instructions and allows the running of all tested SPEC2000 > benchmarks in atomic mode. > > > Diffs > ----- > > configs/common/cpu2000.py ad784e759a74 > src/arch/arm/ArmTLB.py ad784e759a74 > src/arch/arm/SConscript ad784e759a74 > src/arch/arm/faults.hh ad784e759a74 > src/arch/arm/faults.cc ad784e759a74 > src/arch/arm/insts/branch.hh ad784e759a74 > src/arch/arm/insts/branch.cc ad784e759a74 > src/arch/arm/insts/macromem.hh ad784e759a74 > src/arch/arm/insts/macromem.cc PRE-CREATION > src/arch/arm/insts/mem.hh ad784e759a74 > src/arch/arm/insts/mem.cc ad784e759a74 > src/arch/arm/insts/misc.hh PRE-CREATION > src/arch/arm/insts/misc.cc PRE-CREATION > src/arch/arm/insts/mult.hh PRE-CREATION > src/arch/arm/insts/pred_inst.hh ad784e759a74 > src/arch/arm/insts/pred_inst.cc ad784e759a74 > src/arch/arm/insts/static_inst.hh ad784e759a74 > src/arch/arm/insts/static_inst.cc ad784e759a74 > src/arch/arm/insts/vfp.hh PRE-CREATION > src/arch/arm/insts/vfp.cc PRE-CREATION > src/arch/arm/interrupts.hh ad784e759a74 > src/arch/arm/interrupts.cc ad784e759a74 > src/arch/arm/intregs.hh ad784e759a74 > src/arch/arm/isa.hh ad784e759a74 > src/arch/arm/isa.cc PRE-CREATION > src/arch/arm/isa/bitfields.isa ad784e759a74 > src/arch/arm/isa/copyright.txt ad784e759a74 > src/arch/arm/isa/decoder.isa ad784e759a74 > src/arch/arm/isa/decoder/arm.isa PRE-CREATION > src/arch/arm/isa/decoder/decoder.isa PRE-CREATION > src/arch/arm/isa/decoder/thumb.isa PRE-CREATION > src/arch/arm/isa/formats/basic.isa ad784e759a74 > src/arch/arm/isa/formats/branch.isa ad784e759a74 > src/arch/arm/isa/formats/breakpoint.isa PRE-CREATION > src/arch/arm/isa/formats/data.isa PRE-CREATION > src/arch/arm/isa/formats/formats.isa ad784e759a74 > src/arch/arm/isa/formats/fp.isa ad784e759a74 > src/arch/arm/isa/formats/macromem.isa ad784e759a74 > src/arch/arm/isa/formats/mem.isa ad784e759a74 > src/arch/arm/isa/formats/misc.isa PRE-CREATION > src/arch/arm/isa/formats/mult.isa PRE-CREATION > src/arch/arm/isa/formats/pred.isa ad784e759a74 > src/arch/arm/isa/formats/uncond.isa PRE-CREATION > src/arch/arm/isa/formats/unimp.isa ad784e759a74 > src/arch/arm/isa/formats/unknown.isa ad784e759a74 > src/arch/arm/isa/formats/util.isa ad784e759a74 > src/arch/arm/isa/includes.isa ad784e759a74 > src/arch/arm/isa/insts/basic.isa PRE-CREATION > src/arch/arm/isa/insts/branch.isa PRE-CREATION > src/arch/arm/isa/insts/data.isa PRE-CREATION > src/arch/arm/isa/insts/div.isa PRE-CREATION > src/arch/arm/isa/insts/fp.isa PRE-CREATION > src/arch/arm/isa/insts/insts.isa PRE-CREATION > src/arch/arm/isa/insts/ldr.isa PRE-CREATION > src/arch/arm/isa/insts/macromem.isa PRE-CREATION > src/arch/arm/isa/insts/mem.isa PRE-CREATION > src/arch/arm/isa/insts/misc.isa PRE-CREATION > src/arch/arm/isa/insts/mult.isa PRE-CREATION > src/arch/arm/isa/insts/str.isa PRE-CREATION > src/arch/arm/isa/insts/swap.isa PRE-CREATION > src/arch/arm/isa/main.isa ad784e759a74 > src/arch/arm/isa/operands.isa ad784e759a74 > src/arch/arm/isa/templates/basic.isa PRE-CREATION > src/arch/arm/isa/templates/branch.isa PRE-CREATION > src/arch/arm/isa/templates/macromem.isa PRE-CREATION > src/arch/arm/isa/templates/mem.isa PRE-CREATION > src/arch/arm/isa/templates/misc.isa PRE-CREATION > src/arch/arm/isa/templates/mult.isa PRE-CREATION > src/arch/arm/isa/templates/pred.isa PRE-CREATION > src/arch/arm/isa/templates/templates.isa PRE-CREATION > src/arch/arm/isa/templates/vfp.isa PRE-CREATION > src/arch/arm/isa_traits.hh ad784e759a74 > src/arch/arm/linux/linux.hh ad784e759a74 > src/arch/arm/linux/process.hh ad784e759a74 > src/arch/arm/linux/process.cc ad784e759a74 > src/arch/arm/miscregs.hh ad784e759a74 > src/arch/arm/miscregs.cc PRE-CREATION > src/arch/arm/nativetrace.cc ad784e759a74 > src/arch/arm/pagetable.hh ad784e759a74 > src/arch/arm/pagetable.cc ad784e759a74 > src/arch/arm/predecoder.hh ad784e759a74 > src/arch/arm/process.hh ad784e759a74 > src/arch/arm/process.cc ad784e759a74 > src/arch/arm/registers.hh ad784e759a74 > src/arch/arm/table_walker.hh PRE-CREATION > src/arch/arm/table_walker.cc PRE-CREATION > src/arch/arm/tlb.hh ad784e759a74 > src/arch/arm/tlb.cc ad784e759a74 > src/arch/arm/types.hh ad784e759a74 > src/arch/arm/utility.hh ad784e759a74 > src/arch/arm/utility.cc ad784e759a74 > src/arch/isa_parser.py ad784e759a74 > src/base/loader/elf_object.cc ad784e759a74 > src/base/loader/object_file.hh ad784e759a74 > src/cpu/BaseCPU.py ad784e759a74 > src/cpu/exetrace.cc ad784e759a74 > src/cpu/simple/base.cc ad784e759a74 > src/cpu/simple_thread.hh ad784e759a74 > src/dev/arm/SConscript ad784e759a74 > src/dev/arm/Versatile.py ad784e759a74 > src/dev/arm/versatile.hh ad784e759a74 > src/dev/arm/versatile.cc ad784e759a74 > src/dev/copy_engine.cc ad784e759a74 > src/dev/io_device.hh ad784e759a74 > src/dev/io_device.cc ad784e759a74 > src/sim/process.cc ad784e759a74 > tests/quick/00.hello/ref/arm/linux/simple-atomic/config.ini ad784e759a74 > tests/quick/00.hello/ref/arm/linux/simple-atomic/simerr ad784e759a74 > tests/quick/00.hello/ref/arm/linux/simple-atomic/simout ad784e759a74 > tests/quick/00.hello/ref/arm/linux/simple-atomic/stats.txt ad784e759a74 > util/statetrace/arch/tracechild_arm.hh ad784e759a74 > util/statetrace/arch/tracechild_arm.cc ad784e759a74 > util/statetrace/statetrace.cc ad784e759a74 > > Diff: http://reviews.m5sim.org/r/20/diff > > > Testing > ------- > > > Thanks, > > Ali > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
