O3: Clean up the O3 structures and try to pack them a bit better.

Information
Submitter:	Ali Saidi
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 9037:350f47d32f56
---------------------------
O3: Clean up the O3 structures and try to pack them a bit better.

DynInst is extremely large the hope is that this re-organization will put the
most used members close to each other.

Testing Done

Issue Summary

Description	From	Last Updated	Status
Should we hard code this value of 3?	Nilay Vaish	June 1, 2012, 7:33 a.m.	Resolved
Where is this limit of 3 coming from?	Nilay Vaish	June 1, 2012, 7:33 a.m.	Resolved
You probably want to put a comment here explaining that these are offsets from fetchTick and that's why they can ...	Gabe Black	May 31, 2012, 11:35 p.m.	Resolved
This may not be enough. In any case, an exact value could be calculated by the ISA parser like it ...	Gabe Black	June 1, 2012, 7:34 a.m.	Resolved
Nothing for dests? That's odd, but not unbelievable.	Gabe Black	June 1, 2012, 7:34 a.m.	Resolved

Ali, can report the size of the BaseDynInst class before and after
the application of your patch?

Ali Saidi May 30, 2012, 8:39 a.m. (May 30, 2012, 8:39 a.m.)
```
696 bytes before; 512 after for ARM. 
```

Nilay Vaish May 31, 2012, 7:56 a.m. (May 31, 2012, 7:56 a.m.)

That's a lot of improvement. Did you notice improvement in the performance
of any regression / benchmark tests? How did you go about rearranging
the variables of the class? Is the improvement also coming from eliminating
some of the variables?

Ali Saidi June 1, 2012, 7:33 a.m. (June 1, 2012, 7:33 a.m.)

There seemed to be some small benefit. I re-arranged them based on my best guess about which variables would be most commonly accessed and which would be done together. I'm sure there is a better arrangement, but this is the best I could do without some tools to help. It also cleans up the code to put al the variables in one places which makes this a good be easier in the future.

Ship It!

src/cpu/o3/dyn_inst.hh (Diff revision 1)

Should we hard code this value of 3?

Show all issues

Ali Saidi May 31, 2012, 11:38 p.m. (May 31, 2012, 11:38 p.m.)

I'm fine with it. It was experimentally determined and an assert will catch it.

Ali Saidi June 1, 2012, 7:33 a.m. (June 1, 2012, 7:33 a.m.)
```
will change the isa parser
```

src/cpu/o3/dyn_inst.hh (Diff revision 1)

Where is this limit of 3 coming from?

Show all issues

src/cpu/o3/dyn_inst.hh (Diff revision 1)

You probably want to put a comment here explaining that these are offsets from fetchTick and that's why they can be smaller.

Show all issues

Ali Saidi May 31, 2012, 11:38 p.m. (May 31, 2012, 11:38 p.m.)
```
yup
```

src/cpu/o3/dyn_inst.hh (Diff revision 1)

This may not be enough. In any case, an exact value could be calculated by the ISA parser like it does for total sources and dests.

Show all issues

Ali Saidi May 31, 2012, 11:38 p.m. (May 31, 2012, 11:38 p.m.)

any chance you could whip up the code for it? It's not straight forward since we don't account for misc register dests by itself.

Ali Saidi June 1, 2012, 7:34 a.m. (June 1, 2012, 7:34 a.m.)

it actually is, we just keep track of the as isControl not isMisc. It's fixed.

src/cpu/o3/rename_impl.hh (Diff revision 1)

Nothing for dests? That's odd, but not unbelievable.

Show all issues

Ali Saidi May 31, 2012, 11:38 p.m. (May 31, 2012, 11:38 p.m.)

dests is used. It might be removable, but i didn't do it.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Status: Closed (submitted)