bump

src/arch/x86/decoder.hh (Diff revision 1)

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.

Show all issues

1. Gabe Black May 28, 2012, 1:32 p.m. (May 28, 2012, 1:32 p.m.)

   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.

2. Steve Reinhardt May 29, 2012, 1:31 a.m. (May 29, 2012, 1:31 a.m.)

   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.