> On May 28, 2012, 8:05 p.m., Steve Reinhardt wrote: > > src/arch/x86/decoder.hh, line 65 > > <http://reviews.gem5.org/r/1220/diff/1/?file=26857#file26857line65> > > > > I'm kind of confused by your data structure here... why have a separate > > 'chunks' variable instead of using bytes.size() (which I think is what > > chunks is tracking, no?)? And does 'masks' ever represent anything other > > than a contiguous sequence of bytes? > > > > Intuitively I'd think the cache entry would be something like > > std::pair<StaticInstPtr, std::vector<uint8_t> >, and I'm not following why > > it's so much more than that. > > Gabe Black wrote: > An instruction will be contiguous in memory (thankfully), but it doesn't > necessarily start or end on the boundaries between MachInsts which are > currently 8 bytes for x86. The masks let you pull out and compare against the > bytes of interest without having to shift things around or figure out how > many bytes are from this MachInst vs. the next one, etc. The masks can be > computed once when the entry is established and used many times (or at least > that's the idea), and having a value to and against leaving everything in > place is nice and quick. I especially don't want to do things as a series of > bytes because then you'd have to compose them together into a single value to > compare against, find a mask to get just the incoming bytes you wanted, shift > things to get them to line up, etc. > > The "chunks" variable is indeed tracking the number of elements in the > bytes vector. I thought it might be faster to keep track of it locally as a > simple variable and not have to reach into the bytes vector with a function, > but it wouldn't surprise me if there was no actual benefit to that.
OK, I hadn't thought about the alignment issue. That makes sense. Can you add some comments to the data structure to explain what's going on, and what all the fields are? If you can get rid of 'chunks' and just use bytes.size(), I would suggest that. Also, 'bytes' may not be the greatest name, since these are actually words/chunks and not bytes. Also, this can wait for a separate patch, but I would really like to see MachInst renamed to something like FetchChunk. I find it confusing, and I even know the history of it... I'm sure it would be totally befuddling to someone new to the code. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1220/#review2845 ----------------------------------------------------------- On May 25, 2012, 3:45 a.m., Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1220/ > ----------------------------------------------------------- > > (Updated May 25, 2012, 3:45 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9025:bfe6203ca3d5 > --------------------------- > X86: Move address based decode caching in front of the predecoder. > > The predecoder in x86 does a lot of work, most of which can be skipped if the > decoder cache is put in front of it. > > > Diffs > ----- > > src/arch/x86/decoder.hh bb25e7646c41 > src/arch/x86/decoder.cc bb25e7646c41 > src/arch/x86/isa.hh bb25e7646c41 > src/arch/x86/isa.cc bb25e7646c41 > > Diff: http://reviews.gem5.org/r/1220/diff/ > > > Testing > ------- > > > Thanks, > > Gabe Black > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
