I went back and checked this out, and this was actually fine. I would have still liked to have determined that before it went in, but the problem I had with it was misguided. Good work ARM folks, and sorry for the hassle.
Gabe On 01/18/11 15:08, Gabe Black wrote: > I'm pretty sure I mentioned twice that this is likely not the right way > to do this. The ARM predecoder should consume the bytes it has, not > ensure through special behavior in fetch that what it needs will always > be there. I haven't tried it yet, but I remember having concerns that > this would break x86 as well. On top of that, there are style problems. > > Gabe > > Matt Horsnell wrote: >> changeset b2c7e56572a4 in /z/repo/m5 >> details: http://repo.m5sim.org/m5?cmd=changeset;node=b2c7e56572a4 >> description: >> O3: Fix some variable length instruction issues with the O3 CPU and ARM >> ISA. >> >> diffstat: >> >> src/arch/arm/predecoder.cc | 15 +++++++++++---- >> src/arch/arm/predecoder.hh | 22 ++++++++++++++++++---- >> src/cpu/o3/fetch_impl.hh | 14 ++++++++++---- >> 3 files changed, 39 insertions(+), 12 deletions(-) >> >> diffs (184 lines): >> >> diff -r d25827665112 -r b2c7e56572a4 src/arch/arm/predecoder.cc >> --- a/src/arch/arm/predecoder.cc Tue Jan 18 16:30:05 2011 -0600 >> +++ b/src/arch/arm/predecoder.cc Tue Jan 18 16:30:05 2011 -0600 >> @@ -74,11 +74,15 @@ >> void >> Predecoder::process() >> { >> + // emi is typically ready, with some caveats below... >> + emiReady = true; >> + >> if (!emi.thumb) { >> emi.instBits = data; >> emi.sevenAndFour = bits(data, 7) && bits(data, 4); >> emi.isMisc = (bits(data, 24, 23) == 0x2 && >> bits(data, 20) == 0); >> + consumeBytes(4); >> DPRINTF(Predecoder, "Arm inst: %#x.\n", (uint64_t)emi); >> } else { >> uint16_t word = (data >> (offset * 8)); >> @@ -86,7 +90,7 @@ >> // A 32 bit thumb inst is half collected. >> emi.instBits = emi.instBits | word; >> bigThumb = false; >> - offset += 2; >> + consumeBytes(2); >> DPRINTF(Predecoder, "Second half of 32 bit Thumb: %#x.\n", >> emi.instBits); >> if (itstate.mask) { >> @@ -105,7 +109,7 @@ >> emi.instBits = (data >> 16) | (data << 16); >> DPRINTF(Predecoder, "All of 32 bit Thumb: %#x.\n", >> emi.instBits); >> - offset += 4; >> + consumeBytes(4); >> if (itstate.mask) { >> emi.itstate = itstate; >> advanceThumbCond(); >> @@ -117,11 +121,13 @@ >> "First half of 32 bit Thumb.\n"); >> emi.instBits = (uint32_t)word << 16; >> bigThumb = true; >> - offset += 2; >> + consumeBytes(2); >> + // emi not ready yet. >> + emiReady = false; >> } >> } else { >> // A 16 bit thumb inst. >> - offset += 2; >> + consumeBytes(2); >> emi.instBits = word; >> // Set the condition code field artificially. >> emi.condCode = COND_UC; >> @@ -159,6 +165,7 @@ >> CPSR cpsr = tc->readMiscReg(MISCREG_CPSR); >> itstate.top6 = cpsr.it2; >> itstate.bottom2 = cpsr.it1; >> + outOfBytes = false; >> process(); >> } >> >> diff -r d25827665112 -r b2c7e56572a4 src/arch/arm/predecoder.hh >> --- a/src/arch/arm/predecoder.hh Tue Jan 18 16:30:05 2011 -0600 >> +++ b/src/arch/arm/predecoder.hh Tue Jan 18 16:30:05 2011 -0600 >> @@ -45,6 +45,8 @@ >> #ifndef __ARCH_ARM_PREDECODER_HH__ >> #define __ARCH_ARM_PREDECODER_HH__ >> >> +#include <cassert> >> + >> #include "arch/arm/types.hh" >> #include "arch/arm/miscregs.hh" >> #include "base/types.hh" >> @@ -61,6 +63,8 @@ >> ExtMachInst emi; >> MachInst data; >> bool bigThumb; >> + bool emiReady; >> + bool outOfBytes; >> int offset; >> ITSTATE itstate; >> >> @@ -70,6 +74,8 @@ >> bigThumb = false; >> offset = 0; >> emi = 0; >> + emiReady = false; >> + outOfBytes = true; >> } >> >> Predecoder(ThreadContext * _tc) : >> @@ -103,16 +109,22 @@ >> moreBytes(0, 0, machInst); >> } >> >> + inline void consumeBytes(int numBytes) >> + { >> + offset += numBytes; >> + assert(offset <= sizeof(MachInst)); >> + if (offset == sizeof(MachInst)) >> + outOfBytes = true; >> + } >> + >> bool needMoreBytes() >> { >> - return sizeof(MachInst) > offset; >> + return outOfBytes; >> } >> >> bool extMachInstReady() >> { >> - // The only way an instruction wouldn't be ready is if this is a >> - // 32 bit ARM instruction that's not 32 bit aligned. >> - return !bigThumb; >> + return emiReady; >> } >> >> int getInstSize() >> @@ -123,9 +135,11 @@ >> //This returns a constant reference to the ExtMachInst to avoid a >> copy >> ExtMachInst getExtMachInst(PCState &pc) >> { >> + assert(emiReady); >> ExtMachInst thisEmi = emi; >> pc.npc(pc.pc() + getInstSize()); >> emi = 0; >> + emiReady = false; >> return thisEmi; >> } >> }; >> diff -r d25827665112 -r b2c7e56572a4 src/cpu/o3/fetch_impl.hh >> --- a/src/cpu/o3/fetch_impl.hh Tue Jan 18 16:30:05 2011 -0600 >> +++ b/src/cpu/o3/fetch_impl.hh Tue Jan 18 16:30:05 2011 -0600 >> @@ -384,7 +384,7 @@ >> { >> ThreadID tid = pkt->req->threadId(); >> >> - DPRINTF(Fetch, "[tid:%u] Waking up from cache miss.\n",tid); >> + DPRINTF(Fetch, "[tid:%u] Waking up from cache miss.\n", tid); >> >> assert(!pkt->wasNacked()); >> >> @@ -1011,7 +1011,7 @@ >> instruction->setThreadState(cpu->thread[tid]); >> >> DPRINTF(Fetch, "[tid:%i]: Instruction PC %#x (%d) created " >> - "[sn:%lli]\n", tid, thisPC.instAddr(), >> + "[sn:%lli].\n", tid, thisPC.instAddr(), >> thisPC.microPC(), seq); >> >> DPRINTF(Fetch, "[tid:%i]: Instruction is: %s\n", tid, >> @@ -1180,7 +1180,6 @@ >> ExtMachInst extMachInst; >> >> extMachInst = predecoder.getExtMachInst(thisPC); >> - pcOffset = 0; >> staticInst = StaticInstPtr(extMachInst, >> thisPC.instAddr()); >> >> @@ -1188,7 +1187,12 @@ >> ++fetchedInsts; >> >> if (staticInst->isMacroop()) >> + { >> curMacroop = staticInst; >> + } >> + else { >> + pcOffset = 0; >> + } >> } else { >> // We need more bytes for this instruction. >> break; >> @@ -1196,8 +1200,10 @@ >> } >> if (curMacroop) { >> staticInst = curMacroop->fetchMicroop(thisPC.microPC()); >> - if (staticInst->isLastMicroop()) >> + if (staticInst->isLastMicroop()) { >> curMacroop = NULL; >> + pcOffset = 0; >> + } >> } >> >> DynInstPtr instruction = >> _______________________________________________ >> m5-dev mailing list >> m5-dev@m5sim.org >> http://m5sim.org/mailman/listinfo/m5-dev >> > _______________________________________________ > m5-dev mailing list > m5-dev@m5sim.org > http://m5sim.org/mailman/listinfo/m5-dev _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
