I'm still not very happy with all the redundant bitfield stuff, even if it is caused by IntRegIndex casting issues. (It looks like the big problem is that IntRegIndex is an enum rather than an integer type... is there a big benefit from that? Clearly the cost is pretty high, in my opinion. Why not replace it with a typedef to an integer type and a bunch of constants, maybe in their own Arm::IntReg namespace if that helps?)
I'd also like to see the extended bits() notation (or something similar) that concatenates bitfields, e.g., replacing expressions like: (bits(data, 15) << 2) | bits(data,11,10) with: bits(data, 15, 15, 11, 10) (or something similar... I'm open to ideas for better syntax). This makes it more obvious that the bitfields are being concatenated and avoids the magic '2' that really represents the width of bits(_, 11, 10) without being obvious about that. There are even cases where another shift gets folded in that are more confusing; for example, I think this: (bits(machInst, 9) << 6) | (bits(machInst, 7, 3) << 1) is much less clear than: bits(machInst, 9, 9, 7, 3) << 1 On the other hand, I'll probably never look at the ARM code again, so if no one else cares then I won't stand in the way of getting this committed. Other than that and the minor thing below, I'm fine... thanks to both of you for making all the changes you did. Steve On Tue, Jun 1, 2010 at 8:39 AM, Steve Reinhardt <[email protected]> wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/20/#review30 > ----------------------------------------------------------- > > > > src/arch/arm/insts/mem.cc > <http://reviews.m5sim.org/r/20/#comment142> > > This is pretty minor, but I think this code is both more compact and > expresses the intent better (for this case and similarly the following ASR > case): > ccprintf(os, " LSR #%d", (shiftAmt == 0) ? 32 : shiftAmt); > > > > - Steve > > > On 2010-05-26 20:13:19, Ali Saidi wrote: >> >> ----------------------------------------------------------- >> This is an automatically generated e-mail. To reply, visit: >> http://reviews.m5sim.org/r/20/ >> ----------------------------------------------------------- >> >> (Updated 2010-05-26 20:13:19) >> >> >> 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 84bd4089958b >> src/arch/arm/ArmTLB.py 84bd4089958b >> src/arch/arm/SConscript 84bd4089958b >> src/arch/arm/faults.hh 84bd4089958b >> src/arch/arm/faults.cc 84bd4089958b >> src/arch/arm/insts/branch.hh 84bd4089958b >> src/arch/arm/insts/branch.cc 84bd4089958b >> src/arch/arm/insts/macromem.hh 84bd4089958b >> src/arch/arm/insts/macromem.cc PRE-CREATION >> src/arch/arm/insts/mem.hh 84bd4089958b >> src/arch/arm/insts/mem.cc 84bd4089958b >> 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 84bd4089958b >> src/arch/arm/insts/pred_inst.cc 84bd4089958b >> src/arch/arm/insts/static_inst.hh 84bd4089958b >> src/arch/arm/insts/static_inst.cc 84bd4089958b >> src/arch/arm/insts/vfp.hh PRE-CREATION >> src/arch/arm/insts/vfp.cc PRE-CREATION >> src/arch/arm/interrupts.hh 84bd4089958b >> src/arch/arm/interrupts.cc 84bd4089958b >> src/arch/arm/intregs.hh 84bd4089958b >> src/arch/arm/isa.hh 84bd4089958b >> src/arch/arm/isa.cc PRE-CREATION >> src/arch/arm/isa/bitfields.isa 84bd4089958b >> src/arch/arm/isa/copyright.txt 84bd4089958b >> src/arch/arm/isa/decoder.isa 84bd4089958b >> 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 84bd4089958b >> src/arch/arm/isa/formats/branch.isa 84bd4089958b >> 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 84bd4089958b >> src/arch/arm/isa/formats/fp.isa 84bd4089958b >> src/arch/arm/isa/formats/macromem.isa 84bd4089958b >> src/arch/arm/isa/formats/mem.isa 84bd4089958b >> 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 84bd4089958b >> src/arch/arm/isa/formats/uncond.isa PRE-CREATION >> src/arch/arm/isa/formats/unimp.isa 84bd4089958b >> src/arch/arm/isa/formats/unknown.isa 84bd4089958b >> src/arch/arm/isa/formats/util.isa 84bd4089958b >> src/arch/arm/isa/includes.isa 84bd4089958b >> 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 84bd4089958b >> src/arch/arm/isa/operands.isa 84bd4089958b >> 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 84bd4089958b >> src/arch/arm/linux/linux.hh 84bd4089958b >> src/arch/arm/linux/linux.cc 84bd4089958b >> src/arch/arm/linux/process.hh 84bd4089958b >> src/arch/arm/linux/process.cc 84bd4089958b >> src/arch/arm/miscregs.hh 84bd4089958b >> src/arch/arm/miscregs.cc PRE-CREATION >> src/arch/arm/nativetrace.cc 84bd4089958b >> src/arch/arm/pagetable.hh 84bd4089958b >> src/arch/arm/pagetable.cc 84bd4089958b >> src/arch/arm/predecoder.hh 84bd4089958b >> src/arch/arm/predecoder.cc PRE-CREATION >> src/arch/arm/process.hh 84bd4089958b >> src/arch/arm/process.cc 84bd4089958b >> src/arch/arm/registers.hh 84bd4089958b >> src/arch/arm/table_walker.hh PRE-CREATION >> src/arch/arm/table_walker.cc PRE-CREATION >> src/arch/arm/tlb.hh 84bd4089958b >> src/arch/arm/tlb.cc 84bd4089958b >> src/arch/arm/types.hh 84bd4089958b >> src/arch/arm/utility.hh 84bd4089958b >> src/arch/arm/utility.cc 84bd4089958b >> src/arch/isa_parser.py 84bd4089958b >> src/base/loader/elf_object.cc 84bd4089958b >> src/base/loader/object_file.hh 84bd4089958b >> src/cpu/BaseCPU.py 84bd4089958b >> src/cpu/exetrace.cc 84bd4089958b >> src/cpu/simple/base.cc 84bd4089958b >> src/cpu/simple_thread.hh 84bd4089958b >> src/dev/arm/SConscript 84bd4089958b >> src/dev/arm/Versatile.py 84bd4089958b >> src/dev/arm/versatile.hh 84bd4089958b >> src/dev/arm/versatile.cc 84bd4089958b >> src/dev/copy_engine.cc 84bd4089958b >> src/dev/io_device.hh 84bd4089958b >> src/dev/io_device.cc 84bd4089958b >> src/sim/process.cc 84bd4089958b >> tests/quick/00.hello/ref/arm/linux/simple-atomic/config.ini 84bd4089958b >> tests/quick/00.hello/ref/arm/linux/simple-atomic/simerr 84bd4089958b >> tests/quick/00.hello/ref/arm/linux/simple-atomic/simout 84bd4089958b >> tests/quick/00.hello/ref/arm/linux/simple-atomic/stats.txt 84bd4089958b >> util/statetrace/arch/tracechild_arm.hh 84bd4089958b >> util/statetrace/arch/tracechild_arm.cc 84bd4089958b >> util/statetrace/statetrace.cc 84bd4089958b >> >> Diff: http://reviews.m5sim.org/r/20/diff >> >> >> Testing >> ------- >> >> >> Thanks, >> >> Ali >> >> > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
