> On 2010-05-08 11:08:07, Steve Reinhardt wrote: > > src/arch/arm/insts/mem.hh, line 252 > > <http://reviews.m5sim.org/r/20/diff/1/?file=181#file181line252> > > > > seems like this should go in a .cc file > > Gabe Black wrote: > Wouldn't that prevent it from being inlined? Or do we not care since it's > disassembly? > > Steve Reinhardt wrote: > We don't care... disassembly is never performance critical to begin with, > and we only call the methods at most once per StaticInst anyway (the return > string is cached and reused after that). > > Gabe Black wrote: > This may not be the best place to mention it, but there's a glitch in > that that the Florida people had at least attempted to address, although I > wiped out their fix in these changes. Basically, -most- of the disassembly is > constant, but the pc value may change between uses as far as I can tell and > it won't invalidate the disassembled string. > > Steve Reinhardt wrote: > That's not a bug, that's a feature :-). Seriously, it's always been the > case that StaticInst objects are reused for identical instructions appearing > at different PCs. I don't think we've ever done any measurement on it, but > my recollection from other projects is that this is a pretty significant > memory savings over creating a new isntance for every PC. Expecting that the > PC value would be constant reflects not understanding how it's supposed to > work. Is PC-relative addressing the issue? If it's really that important to > have those values be printed as absolute addresses, we could extend the > disassembly to incorporate PC-relative values in such a way that they do get > recalculated. > > Gabe Black wrote: > The fix was specifically for PC relative branches that print their target > along with a symbol+offset, although the problem is more general than that. > Those branches are pretty common, I think, and I would guess are also used in > the Alpha stuff. > > Note that I'm not advocating making the decode cache on hit on the same > inst at the same PC, although I'm also not arguing the opposite since I don't > know one way or the other. I'm just saying our disassembled string may be > stale and it would be nice to fix that. > > Steve Reinhardt wrote: > Sometimes it helps to actually look at the code... there is a documented > way to deal with this: > http://repo.m5sim.org/m5/file/24379f92cc10/src/cpu/static_inst.hh#l423 > > We can discuss whether that documented method could be improved on, but > there's no "glitch" to be worked around. Not only that, but this code is so > old it predates our revision control (it came in on changeset 2)... I think > that means it pre-dates our switch to bitkeeper.
Yep, you're right. I didn't realize that function was virtual and I didn't look into it too deeply. I saw the Florida guys attempt to fix things, looked at the code a little bit, and made a mental note to do something about it later, later happening to be now. - Gabe ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/20/#review15 ----------------------------------------------------------- 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
