Add Global-Global (gAG) branch predictor

Information
Submitter:	Taylor Lloyd
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

This patch adds a global-global (gAG) branch predictor, used by specifying the "global" predType.
From BranchPredictor, it uses the following variables for configuration: 
 - globalPredictorSize
 - globalCtrBits
 - globalHistoryBits

As such, it may conflict with the ongoing effort to factor out the redundant variable in the predictors.

Testing Done

Prior to submission, this was tested by building build/ARM/gem5.debug, and run against a subset of the SPEC CPU2006 benchmarks. Statistics generated seem valid, and it caused no execution errors.

Issue Summary

Description	From	Last Updated	Status
please put authors on a separate line	Ali Saidi	July 15, 2013, 11:34 a.m.	Resolved
please put the type on a separate line from the method name.	Ali Saidi	July 15, 2013, 11:34 a.m.	Resolved
the indenting here should be fixed	Ali Saidi	July 15, 2013, 11:34 a.m.	Resolved
please put a space between if and (taken)	Ali Saidi	July 15, 2013, 11:34 a.m.	Resolved
It looks like 2^globalHistoryBits = globalPredictorSize. Since globalHistoryBits was recently removed from the tournament predictor, it might be better to ...	Erik Tomusk	July 15, 2013, 11:34 a.m.	Resolved
This could walk off the array (see above comment).	Erik Tomusk	July 15, 2013, 11:34 a.m.	Resolved
These updates can also walk off the array (see above).	Erik Tomusk	July 15, 2013, 11:34 a.m.	Resolved
Is this right?	Andreas Hansson	July 15, 2013, 2:16 p.m.	Open
Pardon me for asking something that perhaps is very obvious, but why do we use a void* and not something ...	Andreas Hansson	July 15, 2013, 2:16 p.m.	Open
Same as above	Andreas Hansson	July 15, 2013, 2:16 p.m.	Open
base/intmath has log2 if that helps...	Andreas Hansson	July 15, 2013, 2:16 p.m.	Open
Why not just globalCtrs(globalPredictorSize) in the initialization list?	Andreas Hansson	July 15, 2013, 2:16 p.m.	Open
Whitespace	Andreas Hansson	July 15, 2013, 2:16 p.m.	Open
Whitespace	Andreas Hansson	July 15, 2013, 2:16 p.m.	Open
A comment here elaborating on when/where this is deleted would be fantastic	Andreas Hansson	July 15, 2013, 2:16 p.m.	Open
Same as above. A comment on when/where it's deleted would be great	Andreas Hansson	July 15, 2013, 2:16 p.m.	Open
if _space_ ( Also whitespace at end of line	Andreas Hansson	July 15, 2013, 2:16 p.m.	Open
NULL please :-)	Andreas Hansson	July 15, 2013, 2:16 p.m.	Open
Whitespace	Andreas Hansson	July 15, 2013, 2:16 p.m.	Open

Hi Taylor,

It looks quite good and is very well documented. Thanks! There are a couple of gem5 style things that we need to correct. If you want to take care of them that is great, if not whomever commits the code can do it. I've commented on an instance of these below.

Thanks again,
Ali

src/cpu/pred/global.cc (Diff revision 1)

please put authors on a separate line

Show all issues

src/cpu/pred/global.cc (Diff revision 1)

please put the type on a separate line from the method name.

Show all issues

src/cpu/pred/global.cc (Diff revision 1)

the indenting here should be fixed

Show all issues

src/cpu/pred/global.cc (Diff revision 1)

please put a space between if and (taken)

Show all issues

Looks like a very useful patch. I think it just needs the predictor size redundancy removed.

src/cpu/pred/global.cc (Diff revision 1)

It looks like 2^globalHistoryBits = globalPredictorSize. Since globalHistoryBits was recently removed from the tournament predictor, it might be better to remove it as a parameter here as well and determine it from globalPredictorSize. Alternatively, the relationship between the two parameters must be enforced to avoid seg faults.

Show all issues

src/cpu/pred/global.cc (Diff revision 1)

This could walk off the array (see above comment).

Show all issues

src/cpu/pred/global.cc (Diff revision 1)

These updates can also walk off the array (see above).

Show all issues

Taylor,

Do you think it could spend a few minutes addressing the issues in these reviews? It would be great to get this committed.

Thanks,
Ali

Taylor,

Do you think it could spend a few minutes addressing the issues in these reviews? It would be great to get this committed.

Thanks,
Ali

Taylor,

Do you think it could spend a few minutes addressing the issues in these reviews? It would be great to get this committed.

Thanks,
Ali

Change Summary:

Resolved all issues raised to date. 
    - Reformatted source to match guidelines
    - No longer respects globalHistoryBits, but rather calculates them from globalPredictorSize

Diff:

Revision 2 (+295 -1)

Show changes

	src/cpu/pred/BranchPredictor.py
	src/cpu/pred/SConscript
	src/cpu/pred/bpred_unit.cc
	src/cpu/pred/global.hh
	src/cpu/pred/global.cc

src/cpu/pred/global.hh (Diff revision 2)

Is this right?

Show all issues

src/cpu/pred/global.hh (Diff revision 2)

Pardon me for asking something that perhaps is very obvious, but why do we use a void* and not something more...useful?

Show all issues

src/cpu/pred/global.cc (Diff revision 2)

Same as above

Show all issues

src/cpu/pred/global.cc (Diff revision 2)

base/intmath has log2 if that helps...

Show all issues

src/cpu/pred/global.cc (Diff revision 2)

Why not just globalCtrs(globalPredictorSize) in the initialization list?

Show all issues

src/cpu/pred/global.cc (Diff revision 2)

Whitespace

Show all issues

src/cpu/pred/global.cc (Diff revision 2)

Whitespace

Show all issues

src/cpu/pred/global.cc (Diff revision 2)

A comment here elaborating on when/where this is deleted would be fantastic

Show all issues

src/cpu/pred/global.cc (Diff revision 2)

Same as above. A comment on when/where it's deleted would be great

Show all issues

src/cpu/pred/global.cc (Diff revision 2)

if _space_ (

Also whitespace at end of line

Show all issues

src/cpu/pred/global.cc (Diff revision 2)

NULL please :-)

Show all issues

src/cpu/pred/global.cc (Diff revision 2)

Whitespace

Show all issues

Thanks a million for the changes. It would be great if the last little bit of formatting can be fixed up, and then I'd say it's all ready to go.

There are some utility functions that would make this code slightly more readable. I'm just comparing to tournament.cc

Otherwise, I think it's good.

src/cpu/pred/global.cc (Diff revisions 1 - 2)

It might be easier to do globalHistoryBits=ceilLog2(globalPredictorSize);

src/cpu/pred/global.cc (Diff revision 1)

could do globalHistoryMask = mask(globalHistoryBits); (will need to include "base/bitfield.hh")

You have a pending review.

Review Board 2.0.15

Screenshots

Files

Issue Summary

Change Summary:

Diff: