On Thu, May 24, 2012 at 12:43 AM, Gabe Black <[email protected]> wrote:
> On 05/21/12 15:14, Steve Reinhardt wrote: > > On Mon, May 21, 2012 at 3:48 AM, Gabe Black <[email protected]> > wrote: > > > >> I'll move over to email since this is turning into a discussion more > than > >> review feedback. It's not the first thing on my agenda, but fairly high > up > >> is going to be simplifying this interface at least a little bit. I did > some > >> grepping, and I think the only place getExtMachInst is used is in the > >> checker right now. > >> > > Can that usage be easily eliminated? That's one of my main comments on > > this patch: you eliminated it in a couple of other places (like > > simple/base.cc, inorder/resources/fetch_unit.cc, and legiontrace.cc) and > it > > wasn't clear to me why it wasn't equally easy to eliminate it in the > > checker. If you did that, then we could get rid of getExtMachInst() > > entirely (or at least make it provate to the decoder). > > Sort of. What the checker is doing is somewhat broken. It's trying to > compare the underlying binary version of the two instructions, I presume > to make sure they're the same thing. It does that by reading out the > ExtMachInst with getExtMachInst, but it then assigns that to a MachInst. > Later on it gets the machInst member out of the StaticInst, casts it to > a MachInst as well, and compares that against the first one. There are > at least two problems with this. First, it assumes you *can* cast an > ExtMachInst into a MachInst. This is why there are #ifdefs which exclude > that code for x86. Second, it assumes that by casting down to a > MachInst, you still get relevant information about whether two > instructions are the same. There's a good chance that's true, but > there's no guarantee, and it also throws away the contextualizing state. > > One thing we could do to "fix" the problem, which would itself be a bit > broken, would be to compare the StaticInstPtrs from the two decoders. > Because we presumably have a cache that will return the same > StaticInstPtr for the same input, if they're the same instruction they > *should* end up as the same object in the end, but that's a pretty > fragile assumption. > > Another option would be to eliminate that check entirely. > Thanks for the explanation. My gut reaction is that I doubt the checker found a lot of errors where both the main cpu and the checker fetched from the same PC but got different bits back from the memory system... so as long as the PCs are being compared, there's no need to break good abstractions just to provide backward compatibility with a likely unnecessary check. The StaticInstPtr comparison sounds reasonable to me. One enhancement would be to make a StaticInstPtr mismatch a warning rather than an error, so if someone did decide to randomly flush the decode cache it wouldn't break things, but OTOH if there was a real divergence then the evidence would be there that this is likely a place where it might have started. Steve _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
