Mitch Hayenga got review request #2000!
mem: Extend prefetcher with options and to work on non-block aligned addresses (prefetcher patch #1)
Review Request #2000 - Created Sept. 6, 2013 and submitted
| Information | |
|---|---|
| Mitch Hayenga | |
| gem5 | |
| Reviewers | |
| Default | |
Extends the classic prefetcher. This patch, originally started by Amin Farmahini, extends the classic prefetcher to work on non-block aligned addresses. Because the existing prefetchers in gem5 mask off the lower address bits of cache accesses, many predictable strides fail to be detected. For example, if a load were to stride by 48 bytes, with 64 byte cachelines, the current stride based prefetcher would see an access pattern of 0, 64, 64, 128, 192.... Thus not detecting a constant stride pattern. This patch fixes this, by training the prefetcher on access and not masking off the lower address bits. It also adds the following configuration options: 1) Training/prefetching only on cache misses, 2) Training/prefetching only on data acceses, 3) Optionally tagging prefetches with a PC address. #3 allows prefetchers to train off of prefetch requests in systems with multiple cache levels and PC-based prefetchers present at multiple levels. It also effectively allows a pipelining of prefetch requests (like in POWER4) across multiple levels of cache hierarchy. Improves performance on my gem5 configuration by 4.3% for SPECINT and 4.7% for SPECFP (geomean).
Benchmarked SPEC2006 simpoints, will post on the dev mailing list with more detailed results.
Issue Summary
4
4
0
0
| Description | From | Last Updated | Status |
|---|---|---|---|
| these three could even be const | Andreas Hansson | Sept. 12, 2013, 12:42 p.m. | Open |
| Perhaps it's just me, but all the negation makes my head spin. Could you add a line of comments trying ... | Andreas Hansson | Sept. 12, 2013, 12:42 p.m. | Open |
| ..., aligned to a cache line boundary | Andreas Hansson | Sept. 12, 2013, 12:42 p.m. | Open |
| Should this really be a 0? should it not be the name of an enum? | Andreas Hansson | Sept. 12, 2013, 12:42 p.m. | Open |
Posted (Sept. 6, 2013, 10:48 a.m.)
I am really happy to see this patch submitted. Thanks for posting this. I think we discussed it back in Spring or Winter, but I'll go over it again this weekend. Thanks!
Posted (Sept. 12, 2013, 12:43 p.m.)
-
src/mem/cache/prefetch/base.hh (Diff revision 2) -
these three could even be const
-
src/mem/cache/prefetch/base.cc (Diff revision 2) -
Perhaps it's just me, but all the negation makes my head spin. Could you add a line of comments trying to sum up what conditions we are checking here.
-
src/mem/cache/prefetch/base.cc (Diff revision 2) -
..., aligned to a cache line boundary
-
src/mem/cache/prefetch/base.cc (Diff revision 2) -
Should this really be a 0? should it not be the name of an enum?
Some minor comments, that's all.
seems good... i'd like to see andreas' comments addressed
Review request changed
Updated (Oct. 8, 2013, 6:42 a.m.)
Change Summary:
1) Commented some more 2) Undid the attempt to pass lower bits to lower level caches 3) Made parameters const
Diff: |
Revision 3 (+64 -16) |
|---|
The only thing remaining is the formatting of the summary line, pick a keyword from http://gem5.org/Commit_Access (I guess mem), and keep the line < 65 char
Ship It!