Discussion:
[gem5-dev] Review Request: TournamentBP: Fix some bugs with table sizes and counters
(too old to reply)
Erik Tomusk
2012-10-29 11:07:48 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------

Review request for Default.


Description
-------

Changeset 9355:eb19f07b2afc
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.

There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.


Diffs
-----

src/cpu/pred/tournament.hh e71f71ce233a
src/cpu/pred/tournament.cc e71f71ce233a

Diff: http://reviews.gem5.org/r/1523/diff/


Testing
-------

Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).


Thanks,

Erik Tomusk
Erik Tomusk
2012-10-29 16:26:06 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------

(Updated Oct. 29, 2012, 9:26 a.m.)


Review request for Default.


Description (updated)
-------

Changeset 9356:55cf0b4cddc8
---------------------------
TournamentBP: Fix some bugs with table sizes and counters


Diffs (updated)
-----

src/cpu/pred/tournament.hh e71f71ce233a
src/cpu/pred/tournament.cc e71f71ce233a

Diff: http://reviews.gem5.org/r/1523/diff/


Testing
-------

Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).


Thanks,

Erik Tomusk
Ali Saidi
2012-10-29 16:39:02 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3646
-----------------------------------------------------------


Thanks for working on this Erik. We very much appreciate your contribution.

I've got some questions below. Additionally, the quick regressions test very little functionality in the branch predictor. I'd be curious to see how the long regeressions turn out with your changes. There are a couple of places that don't quite seem right to me as far as differentiating between taken and not taken branches.

Thanks,
Ali


src/cpu/pred/tournament.cc
<http://reviews.gem5.org/r/1523/#comment3614>

it would be great if you could remove it then



src/cpu/pred/tournament.cc
<http://reviews.gem5.org/r/1523/#comment3615>

should this be the historyRegisterMask? Couldn't this be larger than the globalHistory was specified?

This doesn't appear to do what the previous implementation did which was signify that the branch was taken (by oring in a 1 in the lsb).

Both the taken and not taken versions are the same which doesn't seem correct.



src/cpu/pred/tournament.cc
<http://reviews.gem5.org/r/1523/#comment3616>

you created the globalHistoryMask but you're not using it here?




src/cpu/pred/tournament.cc
<http://reviews.gem5.org/r/1523/#comment3618>

a temporary variable would be very helpful here



src/cpu/pred/tournament.cc
<http://reviews.gem5.org/r/1523/#comment3617>

it would be great to use a temporary variable here and make this much more readble


- Ali Saidi
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Oct. 29, 2012, 9:26 a.m.)
Review request for Default.
Description
-------
Changeset 9356:55cf0b4cddc8
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
Diffs
-----
src/cpu/pred/tournament.hh e71f71ce233a
src/cpu/pred/tournament.cc e71f71ce233a
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
Thanks,
Erik Tomusk
Erik Tomusk
2012-10-29 17:30:38 UTC
Permalink
Post by Erik Tomusk
Post by Ali Saidi
Thanks for working on this Erik. We very much appreciate your contribution.
I've got some questions below. Additionally, the quick regressions test very little functionality in the branch predictor. I'd be curious to see how the long regeressions turn out with your changes. There are a couple of places that don't quite seem right to me as far as differentiating between taken and not taken branches.
Thanks,
Ali
Am I missing something, or can I not run the long regressions without access to the binaries?

Other comments below.

-Erik
Post by Erik Tomusk
Post by Ali Saidi
src/cpu/pred/tournament.cc, line 92
<http://reviews.gem5.org/r/1523/diff/2/?file=30936#file30936line92>
it would be great if you could remove it then
I can rip one of them out. Is there any preference as to which one?
Post by Erik Tomusk
Post by Ali Saidi
src/cpu/pred/tournament.cc, line 163
<http://reviews.gem5.org/r/1523/diff/2/?file=30936#file30936line163>
should this be the historyRegisterMask? Couldn't this be larger than the globalHistory was specified?
This doesn't appear to do what the previous implementation did which was signify that the branch was taken (by oring in a 1 in the lsb).
Both the taken and not taken versions are the same which doesn't seem correct.
I've changed the meaning of globalHistory slightly from being "history used by the global predictor" to "history stored in the global history register." Both the choice and global predictor use the global history register, so the amount of history in the register shouldn't just be controlled by the global predictor. The historyRegisterMask is based on globalHistoryBits as defined by the user, regardless of how many bits are actually used in the predictors.

The taken/not taken OR is still there (on the previous line).
Post by Erik Tomusk
Post by Ali Saidi
src/cpu/pred/tournament.cc, line 171
<http://reviews.gem5.org/r/1523/diff/2/?file=30936#file30936line171>
you created the globalHistoryMask but you're not using it here?
globalHistoryMask is used in update() and lookup() below where it truncates globalHistory to the number of bits required by the global history table
Post by Erik Tomusk
Post by Ali Saidi
src/cpu/pred/tournament.cc, line 312
<http://reviews.gem5.org/r/1523/diff/2/?file=30936#file30936line312>
a temporary variable would be very helpful here
Good idea. I'll roll that in with removing one of the redundant variables.
Post by Erik Tomusk
Post by Ali Saidi
src/cpu/pred/tournament.cc, line 326
<http://reviews.gem5.org/r/1523/diff/2/?file=30936#file30936line326>
it would be great to use a temporary variable here and make this much more readble
Ditto


- Erik


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3646
-----------------------------------------------------------
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Oct. 29, 2012, 9:26 a.m.)
Review request for Default.
Description
-------
Changeset 9356:55cf0b4cddc8
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
Diffs
-----
src/cpu/pred/tournament.hh e71f71ce233a
src/cpu/pred/tournament.cc e71f71ce233a
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
Thanks,
Erik Tomusk
Erik Tomusk
2012-10-30 15:37:47 UTC
Permalink
Post by Erik Tomusk
Post by Ali Saidi
src/cpu/pred/tournament.cc, line 92
<http://reviews.gem5.org/r/1523/diff/2/?file=30936#file30936line92>
it would be great if you could remove it then
I can rip one of them out. Is there any preference as to which one?
I removed localPredictorSize because I think a user would be more interested in defining how many bits of history to keep rather than how many entries should be in the counter table. The 2bit_local predictor still uses a localPredictorSize value, so I'm not sure how best to remove it from the configuration when the tournament predictor is used.


- Erik


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3646
-----------------------------------------------------------
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Oct. 30, 2012, 8:31 a.m.)
Review request for Default.
Description
-------
Changeset 9356:a2d5d47de2bf
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py e71f71ce233a
src/cpu/inorder/resources/bpred_unit.cc e71f71ce233a
src/cpu/o3/bpred_unit_impl.hh e71f71ce233a
src/cpu/pred/tournament.hh e71f71ce233a
src/cpu/pred/tournament.cc e71f71ce233a
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
Thanks,
Erik Tomusk
Erik Tomusk
2012-10-30 15:31:04 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------

(Updated Oct. 30, 2012, 8:31 a.m.)


Review request for Default.


Description (updated)
-------

Changeset 9356:a2d5d47de2bf
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.

There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.

The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.


Diffs (updated)
-----

configs/common/O3_ARM_v7a.py e71f71ce233a
src/cpu/inorder/resources/bpred_unit.cc e71f71ce233a
src/cpu/o3/bpred_unit_impl.hh e71f71ce233a
src/cpu/pred/tournament.hh e71f71ce233a
src/cpu/pred/tournament.cc e71f71ce233a

Diff: http://reviews.gem5.org/r/1523/diff/


Testing
-------

Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).


Thanks,

Erik Tomusk
Ali Saidi
2012-11-02 17:22:41 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3667
-----------------------------------------------------------


Hi Erik,

Could you rebase this on top of the patch I committed today?

Thanks,
Ali


- Ali Saidi
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Oct. 30, 2012, 8:31 a.m.)
Review request for Default.
Description
-------
Changeset 9356:a2d5d47de2bf
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py e71f71ce233a
src/cpu/inorder/resources/bpred_unit.cc e71f71ce233a
src/cpu/o3/bpred_unit_impl.hh e71f71ce233a
src/cpu/pred/tournament.hh e71f71ce233a
src/cpu/pred/tournament.cc e71f71ce233a
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
Thanks,
Erik Tomusk
Erik Tomusk
2012-11-06 12:13:17 UTC
Permalink
Post by Erik Tomusk
Post by Ali Saidi
Hi Erik,
Could you rebase this on top of the patch I committed today?
Thanks,
Ali
Done

-Erik


- Erik


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3667
-----------------------------------------------------------
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 6, 2012, 4:03 a.m.)
Review request for Default.
Description
-------
Changeset 9387:38c52462af41
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when this patch is applied onto
changeset 9354.
Thanks,
Erik Tomusk
Erik Tomusk
2012-11-06 11:52:28 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------

(Updated Nov. 6, 2012, 3:52 a.m.)


Review request for Default.


Description (updated)
-------

Changeset 9387:38c52462af41
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.

There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.

The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.


Diffs (updated)
-----

configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09

Diff: http://reviews.gem5.org/r/1523/diff/


Testing
-------

Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).


Thanks,

Erik Tomusk
Erik Tomusk
2012-11-06 11:56:30 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------

(Updated Nov. 6, 2012, 3:56 a.m.)


Review request for Default.


Description
-------

Changeset 9387:38c52462af41
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.

There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.

The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.


Diffs (updated)
-----

configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09

Diff: http://reviews.gem5.org/r/1523/diff/


Testing
-------

Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).


Thanks,

Erik Tomusk
Andreas Hansson
2012-11-06 11:58:41 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3668
-----------------------------------------------------------


I'm curious about the testing statement. No regressions should be failing.

- Andreas Hansson
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 6, 2012, 3:56 a.m.)
Review request for Default.
Description
-------
Changeset 9387:38c52462af41
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
Thanks,
Erik Tomusk
Erik Tomusk
2012-11-06 12:12:45 UTC
Permalink
Post by Erik Tomusk
Post by Andreas Hansson
I'm curious about the testing statement. No regressions should be failing.
The tgen-simple-dram regression is failing for me even on a clean gem5 and
has been for a while. It goes through all my RAM (4GB), a good bit of swap
space, and then just hangs.


- Erik


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3668
-----------------------------------------------------------
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 6, 2012, 4:03 a.m.)
Review request for Default.
Description
-------
Changeset 9387:38c52462af41
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when this patch is applied onto
changeset 9354.
Thanks,
Erik Tomusk
Andreas Hansson
2012-11-06 12:53:38 UTC
Permalink
Post by Erik Tomusk
Post by Andreas Hansson
I'm curious about the testing statement. No regressions should be failing.
The tgen-simple-dram regression is failing for me even on a clean gem5 and
has been for a while. It goes through all my RAM (4GB), a good bit of swap
space, and then just hangs.
Hmm, how odd. It works fine on zizzer and my local machines (RHE5 and Ubuntu 10.04). What configuration are you running? Any chance you could give it a go with valgrind?


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3668
-----------------------------------------------------------
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 6, 2012, 4:03 a.m.)
Review request for Default.
Description
-------
Changeset 9387:38c52462af41
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when this patch is applied onto
changeset 9354.
Thanks,
Erik Tomusk
Erik Tomusk
2012-11-06 13:53:16 UTC
Permalink
Post by Erik Tomusk
Post by Andreas Hansson
I'm curious about the testing statement. No regressions should be failing.
The tgen-simple-dram regression is failing for me even on a clean gem5 and
has been for a while. It goes through all my RAM (4GB), a good bit of swap
space, and then just hangs.
Hmm, how odd. It works fine on zizzer and my local machines (RHE5 and Ubuntu 10.04). What configuration are you running? Any chance you could give it a go with valgrind?
I'm on Scientific Linux 6.3 (so effectively RedHat). Kernel version 2.6.32-279 on x86-64.

Unfortunately I don't think I have time to start digging into the dram test. My valgrind-fu is non-existent and I've got a list of more pressing things that I need to look at.


- Erik


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3668
-----------------------------------------------------------
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 6, 2012, 4:03 a.m.)
Review request for Default.
Description
-------
Changeset 9387:38c52462af41
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when this patch is applied onto
changeset 9354.
Thanks,
Erik Tomusk
Andreas Hansson
2012-11-08 00:33:36 UTC
Permalink
Post by Erik Tomusk
Post by Andreas Hansson
I'm curious about the testing statement. No regressions should be failing.
The tgen-simple-dram regression is failing for me even on a clean gem5 and
has been for a while. It goes through all my RAM (4GB), a good bit of swap
space, and then just hangs.
Hmm, how odd. It works fine on zizzer and my local machines (RHE5 and Ubuntu 10.04). What configuration are you running? Any chance you could give it a go with valgrind?
I'm on Scientific Linux 6.3 (so effectively RedHat). Kernel version 2.6.32-279 on x86-64.
Unfortunately I don't think I have time to start digging into the dram test. My valgrind-fu is non-existent and I've got a list of more pressing things that I need to look at.
Found the problem. Will have a patch on the board shortly.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3668
-----------------------------------------------------------
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 7, 2012, 3:24 a.m.)
Review request for Default.
Description
-------
Changeset 9387:942ab8031852
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when this patch is applied onto
changeset 9354.
Thanks,
Erik Tomusk
Erik Tomusk
2012-11-06 12:03:16 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------

(Updated Nov. 6, 2012, 4:03 a.m.)


Review request for Default.


Changes
-------

Rebase


Description
-------

Changeset 9387:38c52462af41
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.

There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.

The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.


Diffs
-----

configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09

Diff: http://reviews.gem5.org/r/1523/diff/


Testing (updated)
-------

Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).

ARM SE long regressions all pass when this patch is applied onto
changeset 9354.


Thanks,

Erik Tomusk
Ali Saidi
2012-11-06 13:25:52 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3672
-----------------------------------------------------------


Thanks Erik

This seems like a huge improvement. If you could fix up the couple of issue that would be great and we can get it committed.


src/cpu/pred/tournament.cc
<http://reviews.gem5.org/r/1523/#comment3639>

could you switch these to use mask() instead af creating the mask yourself? It just makes it a bit clearer about what is going on.

src/base/bitfields.hh





src/cpu/pred/tournament.cc
<http://reviews.gem5.org/r/1523/#comment3640>

I think something like
globalHistory &= (historyRegisterMask & ~ULL(1)) would be clearer that you're clearing the lowest bit. With a subtract it's not completely clear to me



src/cpu/pred/tournament.cc
<http://reviews.gem5.org/r/1523/#comment3641>

shouldn't need ()



- Ali Saidi
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 6, 2012, 4:03 a.m.)
Review request for Default.
Description
-------
Changeset 9387:38c52462af41
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when this patch is applied onto
changeset 9354.
Thanks,
Erik Tomusk
Erik Tomusk
2012-11-06 19:46:01 UTC
Permalink
Post by Erik Tomusk
src/cpu/pred/tournament.cc, line 112
<http://reviews.gem5.org/r/1523/diff/5/?file=31001#file31001line112>
could you switch these to use mask() instead af creating the mask yourself? It just makes it a bit clearer about what is going on.
src/base/bitfields.hh
How should bitfields.hh be included in the build process? It's not obvious to me how SCons finds headers.


- Erik


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3672
-----------------------------------------------------------
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 6, 2012, 4:03 a.m.)
Review request for Default.
Description
-------
Changeset 9387:38c52462af41
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when this patch is applied onto
changeset 9354.
Thanks,
Erik Tomusk
Ali Saidi
2012-11-07 00:12:10 UTC
Permalink
Post by Erik Tomusk
src/cpu/pred/tournament.cc, line 112
<http://reviews.gem5.org/r/1523/diff/5/?file=31001#file31001line112>
could you switch these to use mask() instead af creating the mask yourself? It just makes it a bit clearer about what is going on.
src/base/bitfields.hh
How should bitfields.hh be included in the build process? It's not obvious to me how SCons finds headers.
you should be able to include it with:
#include "base/bitfields.hh"


- Ali


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3672
-----------------------------------------------------------
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 6, 2012, 4:03 a.m.)
Review request for Default.
Description
-------
Changeset 9387:38c52462af41
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when this patch is applied onto
changeset 9354.
Thanks,
Erik Tomusk
Erik Tomusk
2012-11-07 11:40:22 UTC
Permalink
Post by Erik Tomusk
src/cpu/pred/tournament.cc, line 112
<http://reviews.gem5.org/r/1523/diff/5/?file=31001#file31001line112>
could you switch these to use mask() instead af creating the mask yourself? It just makes it a bit clearer about what is going on.
src/base/bitfields.hh
How should bitfields.hh be included in the build process? It's not obvious to me how SCons finds headers.
#include "base/bitfields.hh"
It was just a typo.


- Erik


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3672
-----------------------------------------------------------
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 7, 2012, 3:24 a.m.)
Review request for Default.
Description
-------
Changeset 9387:942ab8031852
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when this patch is applied onto
changeset 9354.
Thanks,
Erik Tomusk
Erik Tomusk
2012-11-07 11:24:25 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------

(Updated Nov. 7, 2012, 3:24 a.m.)


Review request for Default.


Description (updated)
-------

Changeset 9387:942ab8031852
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.

There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.

The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.


Diffs (updated)
-----

configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09

Diff: http://reviews.gem5.org/r/1523/diff/


Testing
-------

Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).

ARM SE long regressions all pass when this patch is applied onto
changeset 9354.


Thanks,

Erik Tomusk
Erik Tomusk
2012-11-07 11:44:32 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3681
-----------------------------------------------------------



src/cpu/pred/tournament.cc
<http://reviews.gem5.org/r/1523/#comment3647>

This definitely changes behavior, but I think it's a correct change to make. The original should have been (1 << floorPow2(localPredictorSize)) - 1.


- Erik Tomusk
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 7, 2012, 3:24 a.m.)
Review request for Default.
Description
-------
Changeset 9387:942ab8031852
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when this patch is applied onto
changeset 9354.
Thanks,
Erik Tomusk
Erik Tomusk
2012-11-09 10:42:24 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------

(Updated Nov. 9, 2012, 2:42 a.m.)


Review request for Default.


Description
-------

Changeset 9387:942ab8031852
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.

There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.

The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.


Diffs
-----

configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09

Diff: http://reviews.gem5.org/r/1523/diff/


Testing (updated)
-------

Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).

ARM SE long regressions all pass when version 6 of this patch is
applied onto changeset 9385.


Thanks,

Erik Tomusk
Ali Saidi
2012-11-10 04:29:59 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3694
-----------------------------------------------------------

Ship it!


Ship It!

- Ali Saidi
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 9, 2012, 2:42 a.m.)
Review request for Default.
Description
-------
Changeset 9387:942ab8031852
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when version 6 of this patch is
applied onto changeset 9385.
Thanks,
Erik Tomusk
Andreas Hansson
2012-11-10 15:18:49 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3695
-----------------------------------------------------------



src/cpu/pred/tournament.cc
<http://reviews.gem5.org/r/1523/#comment3651>

Is this clearing the LSB? If so perhaps unify this and others to be & ~ULL(1) instead?



src/cpu/pred/tournament.cc
<http://reviews.gem5.org/r/1523/#comment3650>

clang static analyzer rightfully points out that this is undefined as the 1 is not typed (according to the standard)

ULL(1) instead of 1 will fix the warning

Not sure any compiler would ever not do this...but might as well.


- Andreas Hansson
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 9, 2012, 2:42 a.m.)
Review request for Default.
Description
-------
Changeset 9387:942ab8031852
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when version 6 of this patch is
applied onto changeset 9385.
Thanks,
Erik Tomusk
Erik Tomusk
2012-11-12 13:18:22 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------

(Updated Nov. 12, 2012, 5:18 a.m.)


Review request for Default.


Description (updated)
-------

Changeset 9388:1175213b3f61
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.

There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.

The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.


Diffs (updated)
-----

configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09

Diff: http://reviews.gem5.org/r/1523/diff/


Testing
-------

Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).

ARM SE long regressions all pass when version 6 of this patch is
applied onto changeset 9385.


Thanks,

Erik Tomusk
Andreas Hansson
2012-11-12 13:21:39 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/#review3696
-----------------------------------------------------------

Ship it!


Ship It!

- Andreas Hansson
Post by Erik Tomusk
-----------------------------------------------------------
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------
(Updated Nov. 12, 2012, 5:18 a.m.)
Review request for Default.
Description
-------
Changeset 9388:1175213b3f61
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Diffs
-----
configs/common/O3_ARM_v7a.py 4229aedfdd09
src/cpu/inorder/resources/bpred_unit.cc 4229aedfdd09
src/cpu/o3/bpred_unit_impl.hh 4229aedfdd09
src/cpu/pred/tournament.hh 4229aedfdd09
src/cpu/pred/tournament.cc 4229aedfdd09
Diff: http://reviews.gem5.org/r/1523/diff/
Testing
-------
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when version 6 of this patch is
applied onto changeset 9385.
Thanks,
Erik Tomusk
Erik Tomusk
2012-11-12 13:35:02 UTC
Permalink
localPredictorSize still shows up in config.ini even though it's not
used. The value is still required for the 2bit_local predictor, and I
haven't found a place in the code where I can specify that it should not
be listed for the tournament predictor. Any ideas?

-Erik
Post by Erik Tomusk
http://reviews.gem5.org/r/1523/
Ship it!
Ship It!
- Andreas
Review request for Default.
By Erik Tomusk.
/Updated Nov. 12, 2012, 5:18 a.m./
Description
Changeset 9388:1175213b3f61
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Testing
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when version 6 of this patch is
applied onto changeset 9385.
Diffs
* configs/common/O3_ARM_v7a.py (4229aedfdd09)
* src/cpu/inorder/resources/bpred_unit.cc (4229aedfdd09)
* src/cpu/o3/bpred_unit_impl.hh (4229aedfdd09)
* src/cpu/pred/tournament.hh (4229aedfdd09)
* src/cpu/pred/tournament.cc (4229aedfdd09)
View Diff <http://reviews.gem5.org/r/1523/diff/>
Andreas Hansson
2012-11-12 13:41:49 UTC
Permalink
Hi Erik,

There's currently no way of pruning parameters that are not used (besides
creating subclasses which seems like an overkill in this case).

Andreas
Post by Erik Tomusk
localPredictorSize still shows up in config.ini even though it's not
used. The value is still required for the 2bit_local predictor, and I
haven't found a place in the code where I can specify that it should not
be listed for the tournament predictor. Any ideas?
-Erik
Post by Erik Tomusk
http://reviews.gem5.org/r/1523/
Ship it!
Ship It!
- Andreas
Review request for Default.
By Erik Tomusk.
/Updated Nov. 12, 2012, 5:18 a.m./
Description
Changeset 9388:1175213b3f61
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Testing
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when version 6 of this patch is
applied onto changeset 9385.
Diffs
* configs/common/O3_ARM_v7a.py (4229aedfdd09)
* src/cpu/inorder/resources/bpred_unit.cc (4229aedfdd09)
* src/cpu/o3/bpred_unit_impl.hh (4229aedfdd09)
* src/cpu/pred/tournament.hh (4229aedfdd09)
* src/cpu/pred/tournament.cc (4229aedfdd09)
View Diff <http://reviews.gem5.org/r/1523/diff/>
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Erik Tomusk
2012-11-13 14:34:48 UTC
Permalink
In that case, I the patch should be ready.

-Erik
Post by Ali Saidi
Hi Erik,
There's currently no way of pruning parameters that are not used (besides
creating subclasses which seems like an overkill in this case).
Andreas
Post by Erik Tomusk
localPredictorSize still shows up in config.ini even though it's not
used. The value is still required for the 2bit_local predictor, and I
haven't found a place in the code where I can specify that it should not
be listed for the tournament predictor. Any ideas?
-Erik
Post by Erik Tomusk
http://reviews.gem5.org/r/1523/
Ship it!
Ship It!
- Andreas
Review request for Default.
By Erik Tomusk.
/Updated Nov. 12, 2012, 5:18 a.m./
Description
Changeset 9388:1175213b3f61
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.
There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.
The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.
Testing
Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).
ARM SE long regressions all pass when version 6 of this patch is
applied onto changeset 9385.
Diffs
* configs/common/O3_ARM_v7a.py (4229aedfdd09)
* src/cpu/inorder/resources/bpred_unit.cc (4229aedfdd09)
* src/cpu/o3/bpred_unit_impl.hh (4229aedfdd09)
* src/cpu/pred/tournament.hh (4229aedfdd09)
* src/cpu/pred/tournament.cc (4229aedfdd09)
View Diff<http://reviews.gem5.org/r/1523/diff/>
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
gem5-dev mailing list
http://m5sim.org/mailman/listinfo/gem5-dev
--
The University of Edinburgh is a charitable body, registered in
Scotland, with registration number SC005336.
Erik Tomusk
2012-11-20 17:16:56 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1523/
-----------------------------------------------------------

(Updated Nov. 20, 2012, 9:16 a.m.)


Review request for Default.


Description (updated)
-------

Changeset 9394:edeba77c9f2f
---------------------------
TournamentBP: Fix some bugs with table sizes and counters
globalHistoryBits, globalPredictorSize, and choicePredictorSize are decoupled.
globalHistoryBits controls how much history is kept, global and choice
predictor sizes control how much of that history is used when accessing
predictor tables. This way, global and choice predictors can actually be
different sizes, and it is no longer possible to walk off the predictor arrays
and cause a seg fault.

There are now individual thresholds for choice, global, and local saturating
counters, so that taken/not taken decisions are correct even when the
predictors' counters' sizes are different.

The interface for localPredictorSize has been removed from TournamentBP because
the value can be calculated from localHistoryBits.


Diffs (updated)
-----

configs/common/O3_ARM_v7a.py 94383c5124d2
src/cpu/inorder/resources/bpred_unit.cc 94383c5124d2
src/cpu/o3/bpred_unit_impl.hh 94383c5124d2
src/cpu/pred/tournament.hh 94383c5124d2
src/cpu/pred/tournament.cc 94383c5124d2

Diff: http://reviews.gem5.org/r/1523/diff/


Testing
-------

Works with ARM SE quick regression (except for dram target, which fails
with and without this patch).

ARM SE long regressions all pass when version 6 of this patch is
applied onto changeset 9385.


Thanks,

Erik Tomusk

Loading...