The issue here is that we have a significant number of existing checkpoints for large systems that would be prohibitively expensive to regenerate. If it would work to just have the unserialize code default the value to 0 if it's missing, that would be great. It sounds like you are saying that, but could you confirm?
If not, a solution that involves scriptable updates to checkpoint files is OK but not great. In general, we need to be much more cautious about updates that break existing checkpoints. Steve On Fri, Mar 4, 2011 at 2:36 PM, Gabe Black <[email protected]> wrote: > Oh, heh, apparently I'm already serializing it :-P. Well there you go. > The other part still stands. > > Gabe > > On 03/04/11 14:21, Gabe Black wrote: > > That's a good point. I tend to forget about checkpointing since I don't > > ever really use it. To be totally correct we should add that in to the > > serialized state, and since it's not that long since the last time we > > changed things around (yeah, I know, sorry about that) it shouldn't be > > very painful. If we were to set it to 0, then I -think- what would > > happen is either nothing if the pc wasn't a branch and the CPU was > > flushed, or there would be a slight hiccup from a spurious mispredict > > and then the CPU would correct things and continue. > > > > My recommendation is definitely to add it to the checkpoint code (I'll > > try to add it sometime today, but I have a flight) since that minimizes > > hassles in the long term. I think Lisa added the capability to handle a > > field not being present, and if that does actually exist we could use > > that to default to 0. We should add a line to the checkpoints explicitly > > still, although it's probably not feasible to determine what size to use > > without looking at the bytes involved. > > > > Gabe > > > > On 03/04/11 13:14, Beckmann, Brad wrote: > >> Hi Gabe, > >> > >> In this changeset, you added the instruction size to the x86 PCState. > Since checkpoints created prior to this changeset won't include an > instruction size, is it safe to set that value to zero? If not, what can we > do to rectify old checkpoints? > >> > >> Thanks, > >> > >> Brad > >> > >> > >>> -----Original Message----- > >>> From: [email protected] [mailto:[email protected]] > >>> On Behalf Of Gabe Black > >>> Sent: Sunday, February 13, 2011 5:45 PM > >>> To: [email protected] > >>> Subject: [m5-dev] changeset in m5: X86: Only reset npc to reflect > instruction > >>> leng... > >>> > >>> changeset be8762db2561 in /z/repo/m5 > >>> details: http://repo.m5sim.org/m5?cmd=changeset;node=be8762db2561 > >>> description: > >>> X86: Only reset npc to reflect instruction length once. > >>> > >>> When redirecting fetch to handle branches, the npc of the current > pc > >>> state > >>> needs to be left alone. This change makes the pc state record > >>> whether or not > >>> the npc already reflects a real value by making it keep track of > the > >>> current > >>> instruction size, or if no size has been set. > >>> > >>> diffstat: > >>> > >>> src/arch/x86/predecoder.hh | 6 ++++- > >>> src/arch/x86/types.hh | 50 > >>> +++++++++++++++++++++++++++++++++++++++++++++- > >>> 2 files changed, 54 insertions(+), 2 deletions(-) > >>> > >>> diffs (76 lines): > >>> > >>> diff -r 6d955240bb62 -r be8762db2561 src/arch/x86/predecoder.hh > >>> --- a/src/arch/x86/predecoder.hh Sun Feb 13 17:40:07 2011 -0800 > >>> +++ b/src/arch/x86/predecoder.hh Sun Feb 13 17:41:10 2011 -0800 > >>> @@ -225,7 +225,11 @@ > >>> { > >>> assert(emiIsReady); > >>> emiIsReady = false; > >>> - nextPC.npc(nextPC.pc() + getInstSize()); > >>> + if (!nextPC.size()) { > >>> + Addr size = getInstSize(); > >>> + nextPC.size(size); > >>> + nextPC.npc(nextPC.pc() + size); > >>> + } > >>> return emi; > >>> } > >>> }; > >>> diff -r 6d955240bb62 -r be8762db2561 src/arch/x86/types.hh > >>> --- a/src/arch/x86/types.hh Sun Feb 13 17:40:07 2011 -0800 > >>> +++ b/src/arch/x86/types.hh Sun Feb 13 17:41:10 2011 -0800 > >>> @@ -222,7 +222,55 @@ > >>> return true; > >>> } > >>> > >>> - typedef GenericISA::UPCState<MachInst> PCState; > >>> + class PCState : public GenericISA::UPCState<MachInst> > >>> + { > >>> + protected: > >>> + typedef GenericISA::UPCState<MachInst> Base; > >>> + > >>> + uint8_t _size; > >>> + > >>> + public: > >>> + void > >>> + set(Addr val) > >>> + { > >>> + Base::set(val); > >>> + _size = 0; > >>> + } > >>> + > >>> + PCState() {} > >>> + PCState(Addr val) { set(val); } > >>> + > >>> + uint8_t size() const { return _size; } > >>> + void size(uint8_t newSize) { _size = newSize; } > >>> + > >>> + void > >>> + advance() > >>> + { > >>> + Base::advance(); > >>> + _size = 0; > >>> + } > >>> + > >>> + void > >>> + uEnd() > >>> + { > >>> + Base::uEnd(); > >>> + _size = 0; > >>> + } > >>> + > >>> + void > >>> + serialize(std::ostream &os) > >>> + { > >>> + Base::serialize(os); > >>> + SERIALIZE_SCALAR(_size); > >>> + } > >>> + > >>> + void > >>> + unserialize(Checkpoint *cp, const std::string §ion) > >>> + { > >>> + Base::unserialize(cp, section); > >>> + UNSERIALIZE_SCALAR(_size); > >>> + } > >>> + }; > >>> > >>> struct CoreSpecific { > >>> int core_type; > >>> _______________________________________________ > >>> m5-dev mailing list > >>> [email protected] > >>> http://m5sim.org/mailman/listinfo/m5-dev > >> _______________________________________________ > >> m5-dev mailing list > >> [email protected] > >> http://m5sim.org/mailman/listinfo/m5-dev > > _______________________________________________ > > m5-dev mailing list > > [email protected] > > http://m5sim.org/mailman/listinfo/m5-dev > > _______________________________________________ > m5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/m5-dev > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
