Implementation of SubGraph Profile information for the Optimizer.

Implementation of SubGraph Profile information for the Optimizer.

R=chrisha@chromium.org

Committed: https://code.google.com/p/sawbuck/source/detail?r=1920

[chrisha, 2013-11-22 19:57:15]

Yeah, the approach seems sane to me. Unittests!

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
File syzygy/optimize/application_profile.cc (right):

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.cc:263: return false;
return false ?

Also, this doesn't appear like it can fail. Maybe it doesn't need a return type?

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
File syzygy/optimize/application_profile.h (right):

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:82: // @param subgraph the subgraph to
find profile information.
subgraph for which to calculate profiler information.

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:168: const BasicCodeBlock* block) const;
This only needs to be +4 from the beginning of const (the function declaration
starts there).

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:178: // A empty profile used for all basic
block never executed.
An* ... blocks* ...

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:194: // Allow ApplicationProfile to modify
private field.
fields*

[etienneb, 2013-11-26 23:31:02]

PTAnL.

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
File syzygy/optimize/application_profile.cc (right):

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.cc:263: return false;
On 2013/11/22 19:57:15, chrisha wrote:
> return false ?
> 
> Also, this doesn't appear like it can fail. Maybe it doesn't need a return
type?

Done.

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
File syzygy/optimize/application_profile.h (right):

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:82: // @param subgraph the subgraph to
find profile information.
On 2013/11/22 19:57:15, chrisha wrote:
> subgraph for which to calculate profiler information.

Done.

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:168: const BasicCodeBlock* block) const;
On 2013/11/22 19:57:15, chrisha wrote:
> This only needs to be +4 from the beginning of const (the function declaration
> starts there).

Done.

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:178: // A empty profile used for all basic
block never executed.
On 2013/11/22 19:57:15, chrisha wrote:
> An* ... blocks* ...

Done.

https://codereview.appspot.com/30380043/diff/20001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:194: // Allow ApplicationProfile to modify
private field.
On 2013/11/22 19:57:15, chrisha wrote:
> fields*

Done.

[chrisha, 2013-11-28 20:11:12]

Moar nits

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
File syzygy/optimize/application_profile.cc (right):

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.cc:238: &miss_predicted);
mispredicted (here and elsewhere)

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
File syzygy/optimize/application_profile.h (right):

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:152: class SubGraphProfile {
A class description?

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:182: class
SubGraphProfile::BasicBlockProfile {
Ditto.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:194: // Returns the ratio of
miss-prediction to jump to the basic block successors.
mispredictions to jumps to successors.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:196: double GetMissPredictedRatio() const;
Mispredicted

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:198: // Returns the number of time a given
successor was taken from this basic
times*

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:204: // Returns the ratio of branch taken
for a given successor.
branch taken to what? to branch not taken? Be specific.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:216: EntryCountType miss_predicted_;
mispredict, here and everywhere

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:218: // Map successors to the taken count.
Maps*

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
File syzygy/optimize/application_profile_unittest.cc (right):

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile_unittest.cc:19: 
Remove the blank line here.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile_unittest.cc:51: const uint32
kSmallCodeAddress = 0x3000;
RelativeAddress instead of uint32?

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile_unittest.cc:52: const int32
kBasicBlockOffset0 = 0;
Offset instead of int32?

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile_unittest.cc:57: const uint32
kBasicBlockCount2 = 20;
size_t or uint32 for your counts? Be consistent. Or use EntryCountType.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile_unittest.cc:282:
DCHECK_NE(reinterpret_cast<BasicCodeBlock*>(NULL), bb0);
Never use DCHECKs in a unit test. Rather, these should be assertions. And feel
free to simply ASSERT(bb0), without the casting stuff.

[etienneb, 2013-11-28 20:30:39]

PTAnL.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
File syzygy/optimize/application_profile.cc (right):

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.cc:238: &miss_predicted);
On 2013/11/28 20:11:12, chrisha wrote:
> mispredicted (here and elsewhere)

Done.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
File syzygy/optimize/application_profile.h (right):

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:194: // Returns the ratio of
miss-prediction to jump to the basic block successors.
On 2013/11/28 20:11:12, chrisha wrote:
> mispredictions to jumps to successors.

Done.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:196: double GetMissPredictedRatio() const;
On 2013/11/28 20:11:12, chrisha wrote:
> Mispredicted

Done.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:198: // Returns the number of time a given
successor was taken from this basic
On 2013/11/28 20:11:12, chrisha wrote:
> times*

Done.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:204: // Returns the ratio of branch taken
for a given successor.
On 2013/11/28 20:11:12, chrisha wrote:
> branch taken to what? to branch not taken? Be specific.

Done.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:216: EntryCountType miss_predicted_;
On 2013/11/28 20:11:12, chrisha wrote:
> mispredict, here and everywhere

Done.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile.h:218: // Map successors to the taken count.
On 2013/11/28 20:11:12, chrisha wrote:
> Maps*

Done.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
File syzygy/optimize/application_profile_unittest.cc (right):

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile_unittest.cc:19: 
On 2013/11/28 20:11:12, chrisha wrote:
> Remove the blank line here.

Done.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile_unittest.cc:51: const uint32
kSmallCodeAddress = 0x3000;
On 2013/11/28 20:11:12, chrisha wrote:
> RelativeAddress instead of uint32?

Done.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile_unittest.cc:52: const int32
kBasicBlockOffset0 = 0;
On 2013/11/28 20:11:12, chrisha wrote:
> Offset instead of int32?

Done.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile_unittest.cc:57: const uint32
kBasicBlockCount2 = 20;
On 2013/11/28 20:11:12, chrisha wrote:
> size_t or uint32 for your counts? Be consistent. Or use EntryCountType.

Done.

https://codereview.appspot.com/30380043/diff/60001/syzygy/optimize/applicatio...
syzygy/optimize/application_profile_unittest.cc:282:
DCHECK_NE(reinterpret_cast<BasicCodeBlock*>(NULL), bb0);
On 2013/11/28 20:11:12, chrisha wrote:
> Never use DCHECKs in a unit test. Rather, these should be assertions. And feel
> free to simply ASSERT(bb0), without the casting stuff.

Done.

[chrisha, 2013-11-28 21:08:26]

Awesome, lgtm!

[etienneb, 2013-11-28 21:35:07]

On 2013/11/28 21:08:26, chrisha wrote:
> Awesome, lgtm!

thanks, committing.

[etienneb, 2013-11-28 22:04:22]

Committed patchset #5 manually as r1920 (presubmit successful).