> On 2011-09-06 21:33:13, Steve Reinhardt wrote: > > Hi Gabe, > > > > Overall this looks good. Just two comments: > > > > 1. I think we need to have a global per-ISA decode cache. I understand > > that if the decoder becomes stateful then the "front end" decoder object > > needs to be per context, but I'm very concerned that if we replicate the > > entire decoder cache for each context (as it looks like you're doing here) > > that will have a pretty big impact on memory footprint and cache miss rate > > for large target systems. Ideally the per-context "front end" decoders > > could share a global per-ISA "back end" that has most or all of the > > caching. Once we go parallel then there will be some interesting tricks to > > keep the threads coordinated as they update the cache, but I'm willing to > > deal with that when we get there. > > > > 2. Less significantly, the "foo->getDecoderPtr()->decode()" idiom is kind > > of tiring, especially since the only reason anyone ever calls > > getDecoderPtr() in the current code is to call decode() on what comes back. > > I'd prefer to see the objects that have getDecoderPtr() also have a direct > > decode() method that encapsulates the idiom, so you can just call (for > > example) "context->decode()". It just seems a lot cleaner semantically > > too; you want to decode the instruction using this context, and the fact > > that there's a separate embedded decoder object that you need to get a > > pointer to and then dereference is just an implementation detail that > > should be hidden as much as possible. > > > > Steve > > > > Steve Reinhardt wrote: > Oops, somehow I overlooked the 'static' on the DecodeCache object > declaration... so never mind about #1. Sorry. > > I do wonder if it would make more sense to have the 'recent' array > associated with the Decoder rather than the DecodeCache though, since the > locality is most likely per context and not global. That's more of a future > potential optimization than anything that needs to be changed at this point > though.
Yeah, I was going to mention that for #1, but you beat me to it. For #2, the only reason getDecoderPtr is called *now* is to call decode, but the intention is that in the future ISA code could react to state changes that change mode and reconfigure the decoder. That means there still needs to be a getDecoderPtr, and while a specific decode function would be cleaner in a lot of cases, because we have the context code copied over and over, a redundant function adds a lot of extra code without being truly necessary. I'd like to see us get rid of all the at least redundant feeling interface objects, but in the mean time... - Gabe ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/828/#review1495 ----------------------------------------------------------- On 2011-08-16 03:08:30, Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/828/ > ----------------------------------------------------------- > > (Updated 2011-08-16 03:08:30) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > Decode: Pull instruction decoding out of the StaticInst class into its own. > > This change pulls the instruction decoding machinery (including caches) out of > the StaticInst class and puts it into its own class. This has a few intrinsic > benefits. First, the StaticInst code, which has gotten to be quite large, gets > simpler. Second, the code that handles decode caching is now separated out > into its own component and can be looked at in isolation, making it easier to > understand. I took the opportunity to restructure the code a bit which will > hopefully also help. > > Beyond that, this change also lays some ground work for each ISA to have its > own, potentially stateful decode object. We'd be able to include less > contextualizing information in the ExtMachInst objects since that context > would be applied at the decoder. Also, the decoder could "know" ahead of time > that all the instructions it's going to see are going to be, for instance, 64 > bit mode, and it will have one less thing to check when it decodes them. > Because the decode caching mechanism has been separated out, it's now possible > to have multiple caches which correspond to different types of decoding > context. Having one cache for each element of the cross product of different > configurations may become prohibitive, so it may be desirable to clear out the > cache when relatively static state changes and not to have one for each > setting. > > Because the decode function is no longer universally accessible as a static > member of the StaticInst class, a new function was added to the ThreadContexts > that returns the applicable decode object. > > > Diffs > ----- > > src/arch/alpha/remote_gdb.cc 889818c58eff > src/arch/arm/remote_gdb.cc 889818c58eff > src/cpu/SConscript 889818c58eff > src/cpu/decode.hh PRE-CREATION > src/cpu/decode.cc PRE-CREATION > src/cpu/decode_cache.hh PRE-CREATION > src/cpu/inorder/cpu.hh 889818c58eff > src/cpu/inorder/cpu.cc 889818c58eff > src/cpu/inorder/inorder_dyn_inst.hh 889818c58eff > src/cpu/inorder/inorder_dyn_inst.cc 889818c58eff > src/cpu/inorder/resources/fetch_unit.hh 889818c58eff > src/cpu/inorder/resources/fetch_unit.cc 889818c58eff > src/cpu/inorder/thread_context.hh 889818c58eff > src/cpu/legiontrace.cc 889818c58eff > src/cpu/o3/fetch.hh 889818c58eff > src/cpu/o3/fetch_impl.hh 889818c58eff > src/cpu/o3/thread_context.hh 889818c58eff > src/cpu/simple/base.hh 889818c58eff > src/cpu/simple/base.cc 889818c58eff > src/cpu/simple_thread.hh 889818c58eff > src/cpu/static_inst.hh 889818c58eff > src/cpu/static_inst.cc 889818c58eff > src/cpu/thread_context.hh 889818c58eff > > Diff: http://reviews.m5sim.org/r/828/diff > > > Testing > ------- > > > Thanks, > > Gabe > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
