inorder: pcstate and delay slots bug

Information
Submitter:	Korey Sewell
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	ali, gblack, nate, stever

Description

inorder: pcstate and delay slots bug
not taken delay slots were not being advanced correctly to pc+8, so for those ISAs
we 'advance()' the pcstate one more time for the desired effect

Testing Done

Part of the idea of the PCState stuff is that this shouldn't be necessary. When I converted InOrder I fudged things a little since fetch wasn't structured in a way that made things drop into place and I wasn't sure exactly how everything worked, but the details of branch delay slots should be totally hidden. From my commit message:

One drawback of making the StaticInsts advance the PC is that you have to
actually have one to advance the PC. This would, superficially, seem to
require decoding an instruction before fetch could advance. This is, as far as
I can tell, realistic. fetch would advance through memory addresses, not PCs,
perhaps predicting new memory addresses using existing ones. More
sophisticated decisions about control flow would be made later on, after the
instruction was decoded, and handed back to fetch. If branching needs to
happen, some amount of decoding needs to happen to see that it's a branch,
what the target is, etc. This could get a little more complicated if that gets
done by the predecoder, but I'm choosing to ignore that for now.

I imagine fetch moving through memory addresses like I said, and then perhaps decode (or fetch, if it's decoding the instruction) advancing a corresponding PC object that works alongside the StaticInsts being set up. Decode would grab the right bytes out of the fetch buffer depending on the actual value of instAddr(). The fact that branch delay slots don't just go to the next instruction if the delay slot is annulled should be predicted by the branch predictor because it associates a PCState with the next PCState when branching.

It gets a little confusing and I haven't looked at that stuff for a few months, but it worked out for O3.

Korey Sewell Jan. 26, 2011, 11:03 p.m. (Jan. 26, 2011, 11:03 p.m.)

I wish I had the exact error trace that I was debugging in front of me, but I'll give it a go on understanding on then relook at how it affects this patch.

Are we saying the branch predictor should be responsible for updating the NNPC (not taken or taken in the PCState?)

I believe what I was observing is that the PCState before branch prediction is:
PC:  Addr
NPC: Addr+4
NNPC: Addr+4 (?, something that wasnt Addr+8)

But then when an instruction ended up being predicted not-taken, the NNPC wasnt being updated correctly so we break there. This is because the predicted PC is set to default be the not taken path achieved by advancing once on PCState but that doesnt work for the default value when it should advance 2x.

I'll try to mark in the review , the problem case.

(Lastly, was your (Gabe) goal to remove all #ISA_HAS_DELAY_SLOT in the code?)

Gabe Black Jan. 27, 2011, 9:22 a.m. (Jan. 27, 2011, 9:22 a.m.)

You should only be predicting entire PCState objects, not individual components. If the PC doesn't change in the way you expect, then you have a mispredict and you go back and fix things up. The predictor will learn what the entire new PCState looks like after a branch as if it was all one big possibly complex branch target, so it's pc, npc, nnpc, etc will all do what it's supposed to do, possibly after getting it wrong once.

The idea was to get rid of the ISA_HAS_DELAY_SLOT type code, also including all the special handling for all the different components PCs might have and the rules for updating them (branch delay slots, variable length instructions, mode changing, microcode). That stuff was getting really unwieldy because it was trying to do everything with the same blob of code, and it had to be reimplemented for each CPU which made it even more error prone and a lot more work to get going. Now, ideally, x86 or ARM's PC semantics would just drop into InOrder and do what they're supposed to do without having to jump in and do a bunch of remodeling. Nothing's perfect, of course, so there might be some issues, but it seemed to go pretty smoothly for O3 and cleared up some fairly serious otherwise undiscovered performance bugs.

src/cpu/inorder/resources/branch_predictor.cc (Diff revision 1)

Right here, we are trying to set the default not-taken path of the branch. 

We do this, because The Branch Predictor will only update the PCState if it's a taken branch...

Is that violating how you (Gabe) intending PCState to work?

Gabe Black Jan. 27, 2011, 9:15 a.m. (Jan. 27, 2011, 9:15 a.m.)

It gets a little muddy because the branch predictors still have an idea of taken and not taken, and that's ill defined with branch delay slots. If you define "not taken" as continuing execution with the next instruction, branches may never not be taken because they either go to a different target or skip an instruction. I don't remember exactly how this worked out in O3 for SPARC but you could look at that to get an idea of what's going on. Even though O3 doesn't make special accomadations for branch delay slots and annulling, doing things that way gave significantly better simulated performance than trying to handle things directly.

src/cpu/inorder/resources/fetch_seq_unit.cc (Diff revision 1)

This code was trying to set a default prediction BUT it may be unnecessary since we are setting the default prediction (not taken) in the branch predictor any way

Description:

		inorder: pcstate and delay slots bug
~		For ISAs with delay slots, not taken branches were not being advanced correctly to pc+8, so for those ISAs
	~	not taken delay slots were not being advanced correctly to pc+8, so for those ISAs
		we 'advance()' the pcstate one more time for the desired effect

Diff:

Revision 2 (+20 -23)

Show changes

	src/cpu/inorder/resources/bpred_unit.cc
	src/cpu/inorder/resources/branch_predictor.cc
	src/cpu/inorder/resources/fetch_seq_unit.cc

This looks better. It would be even better still if we could get rid of that last ISA_HAS_DELAY_SLOT, but that might be hard to do. In O3 I made fetch walk past skipped instructions and I think I might have even still had an ISA_HAS_DELAY_SLOT in there.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Description:

Diff: