Abcd for performance check.

Abcd for performance check.

[Jakob Kummerow, 2013-04-17 11:34:15]

https://codereview.chromium.org/14046017/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/14046017/diff/1/src/flag-definitions.h#newcod...
src/flag-definitions.h:221: DEFINE_bool(idefs, true, "use informative
definitions")
intentional?

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:205: int direction = decomposition.scale() > 0 ? -1
: 1;
why not just "int direction = -decomposition.scale();"?

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:207: && decomposition.base()->IsRelationTrue(
nit: '&&' on the previous line please

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:208: relation, other, 0, 0)) {
nit: fits on last line?

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1033: : HUnaryOperation(phi), phi_(phi),
relation_(relation),
nit: one line per initializer, please

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1049: == context->upper_bound()->ActualValue()) {
nit: operator on the previous line

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1061: && !block()->graph()->use_optimistic_licm())
return;
If it doesn't make a significant difference, I'd rather be careful than
optimistic here.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1063: bool starting_loop_is_nested_in_my_loop =
false;
we can make this loop a little simpler:
    // See if starting_loop is nested in my_loop.
    while (starting_loop != my_loop) {
      starting_loop = starting_loop->parent_loop();
      if (starting_loop == NULL) return;
    }
or you could just call "my_loop->IsInThisLoop(starting_loop);"

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1077: if (candidate->IsNumericConstraint()) {
for consistency with code below, I'd transform this into:
if (!candidate->IsNumericConstraint()) continue;

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1090: &&
!induction_limit->block()->Dominates(my_pre_header)) continue;
nit: operator on the previous line

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1092: &&
!upper_bound->block()->Dominates(my_pre_header)) continue;
nit: operator on the previous line

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1096: || induction_limit->IsInteger32Constant())) {
nit: operator on the previous line

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1160: } else if (context->upper_bound_guarantee()
!= NULL &&
I think the "else" needs to be removed.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:3229: &&
delta->IsRelationTrue(NumericRelation::Ge(),
nit: operator on the previous line

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:3235: &&
delta->IsRelationTrue(NumericRelation::Le(),
nit: operator on the previous line

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1466: HBasicBlock* other_block = NULL);
I think "other_block" either needs a comment or a more descriptive name.
"jump_target_when_false"?

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1505: : relation_(relation),
other_block_(other_block) {
nit: indentation

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3362: && related_value->IsInteger32Constant()) {
nit: we're pretty consistent about putting binary operators at the end of the
first line.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:4641: && right()->GetInteger32Constant() <= 1) {
As discussed, I think you need an additional check here that
right()->GetInteger32Constant >= 0.

Please check right shifts for the same issue.

And a nit: '&&' on the previous line please.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:4644: // like ((base + offset) >> scale) with one
single decomposition.
s/>>/<</ ?

https://codereview.chromium.org/14046017/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14046017/diff/1/src/hydrogen.cc#newcode455
src/hydrogen.cc:455: while (current_loop != NULL) {
Please add a comment saying that when we support try..catch in crankshaft, we
need another condition here: break the while-loop when encountering the catch
block. From a correctness PoV we're safe anyway, but unnecessarily marking loops
as having additional exits could prevent BCE.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen.cc#newcode3447
src/hydrogen.cc:3447: if (current->IsThrow()) {
Let's move this into the block ordering phase where ProcessEdge is called too.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/14046017/diff/1/src/hydrogen.h#newcode251
src/hydrogen.h:251: bool IsInThisLoop(HLoopInformation* other) {
I'd rename this to IsNestedInThisLoop (or just IsNested).

[Massi, 2013-05-07 11:32:02]

These are the minimal changes to fix all known performance regressions.
The optimistic hoisting is actually necessary to overcome the "GVN identity"
problem (and is not harmful, at least so far).

Of course I intend to set the idefs flag to false before committing (and turn it
on in a separate cl).

https://codereview.chromium.org/14046017/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/14046017/diff/1/src/flag-definitions.h#newcod...
src/flag-definitions.h:221: DEFINE_bool(idefs, true, "use informative
definitions")
On 2013/04/17 11:34:15, Jakob wrote:
> intentional?
Yes and no.
For now it is on for convenience but I intend to switch it off before committing
and turn it on in a separate CL.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:205: int direction = decomposition.scale() > 0 ? -1
: 1;
On 2013/04/17 11:34:15, Jakob wrote:
> why not just "int direction = -decomposition.scale();"?

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:207: && decomposition.base()->IsRelationTrue(
On 2013/04/17 11:34:15, Jakob wrote:
> nit: '&&' on the previous line please

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:208: relation, other, 0, 0)) {
On 2013/04/17 11:34:15, Jakob wrote:
> nit: fits on last line?

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1033: : HUnaryOperation(phi), phi_(phi),
relation_(relation),
On 2013/04/17 11:34:15, Jakob wrote:
> nit: one line per initializer, please

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1049: == context->upper_bound()->ActualValue()) {
On 2013/04/17 11:34:15, Jakob wrote:
> nit: operator on the previous line

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1077: if (candidate->IsNumericConstraint()) {
On 2013/04/17 11:34:15, Jakob wrote:
> for consistency with code below, I'd transform this into:
> if (!candidate->IsNumericConstraint()) continue;

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1090: &&
!induction_limit->block()->Dominates(my_pre_header)) continue;
On 2013/04/17 11:34:15, Jakob wrote:
> nit: operator on the previous line

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1092: &&
!upper_bound->block()->Dominates(my_pre_header)) continue;
On 2013/04/17 11:34:15, Jakob wrote:
> nit: operator on the previous line

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1096: || induction_limit->IsInteger32Constant())) {
On 2013/04/17 11:34:15, Jakob wrote:
> nit: operator on the previous line

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1160: } else if (context->upper_bound_guarantee()
!= NULL &&
On 2013/04/17 11:34:15, Jakob wrote:
> I think the "else" needs to be removed.

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1160: } else if (context->upper_bound_guarantee()
!= NULL &&
On 2013/04/17 11:34:15, Jakob wrote:
> I think the "else" needs to be removed.

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:3229: &&
delta->IsRelationTrue(NumericRelation::Ge(),
On 2013/04/17 11:34:15, Jakob wrote:
> nit: operator on the previous line

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:3235: &&
delta->IsRelationTrue(NumericRelation::Le(),
On 2013/04/17 11:34:15, Jakob wrote:
> nit: operator on the previous line

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1466: HBasicBlock* other_block = NULL);
On 2013/04/17 11:34:15, Jakob wrote:
> I think "other_block" either needs a comment or a more descriptive name.
> "jump_target_when_false"?

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1505: : relation_(relation),
other_block_(other_block) {
On 2013/04/17 11:34:15, Jakob wrote:
> nit: indentation

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:3362: && related_value->IsInteger32Constant()) {
On 2013/04/17 11:34:15, Jakob wrote:
> nit: we're pretty consistent about putting binary operators at the end of the
> first line.

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:4641: && right()->GetInteger32Constant() <= 1) {
On 2013/04/17 11:34:15, Jakob wrote:
> As discussed, I think you need an additional check here that
> right()->GetInteger32Constant >= 0.
> 
> Please check right shifts for the same issue.
> 
> And a nit: '&&' on the previous line please.

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:4644: // like ((base + offset) >> scale) with one
single decomposition.
On 2013/04/17 11:34:15, Jakob wrote:
> s/>>/<</ ?

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14046017/diff/1/src/hydrogen.cc#newcode455
src/hydrogen.cc:455: while (current_loop != NULL) {
On 2013/04/17 11:34:15, Jakob wrote:
> Please add a comment saying that when we support try..catch in crankshaft, we
> need another condition here: break the while-loop when encountering the catch
> block. From a correctness PoV we're safe anyway, but unnecessarily marking
loops
> as having additional exits could prevent BCE.

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen.cc#newcode3447
src/hydrogen.cc:3447: if (current->IsThrow()) {
On 2013/04/17 11:34:15, Jakob wrote:
> Let's move this into the block ordering phase where ProcessEdge is called too.

Done.

https://codereview.chromium.org/14046017/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/14046017/diff/1/src/hydrogen.h#newcode251
src/hydrogen.h:251: bool IsInThisLoop(HLoopInformation* other) {
On 2013/04/17 11:34:15, Jakob wrote:
> I'd rename this to IsNestedInThisLoop (or just IsNested).

Done.

[Jakob Kummerow, 2013-05-15 12:56:35]

LGTM with a few more nits.

Let's discuss the higher-level situation on Friday.

https://codereview.chromium.org/14046017/diff/23001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/14046017/diff/23001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:1204:
!induction_limit->block()->Dominates(my_pre_header)) continue;
another nit: {} please when the condition doesn't fit onto a single line. Again
below.

https://codereview.chromium.org/14046017/diff/23001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2106: HValue* induction_base_cantidate =
OperandAt(other_operand_index);
nit: s/cantidate/candidate/

https://codereview.chromium.org/14046017/diff/23001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/14046017/diff/23001/src/hydrogen-instructions...
src/hydrogen-instructions.h:871: bool result) :
nit: colon goes on the next line, with 4 spaces of indentation relative to
"NumericRelationTableElement(".

https://codereview.chromium.org/14046017/diff/23001/src/hydrogen-instructions...
src/hydrogen-instructions.h:880: NumericRelationTableElement(const
NumericRelationTableElement& other) :
nit: same here

[Massi, 2013-05-21 12:49:56]

https://codereview.chromium.org/14046017/diff/23001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/14046017/diff/23001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:1204:
!induction_limit->block()->Dominates(my_pre_header)) continue;
On 2013/05/15 12:56:35, Jakob wrote:
> another nit: {} please when the condition doesn't fit onto a single line.
Again
> below.

Done.

https://codereview.chromium.org/14046017/diff/23001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2106: HValue* induction_base_cantidate =
OperandAt(other_operand_index);
On 2013/05/15 12:56:35, Jakob wrote:
> nit: s/cantidate/candidate/

Done.

https://codereview.chromium.org/14046017/diff/23001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/14046017/diff/23001/src/hydrogen-instructions...
src/hydrogen-instructions.h:871: bool result) :
On 2013/05/15 12:56:35, Jakob wrote:
> nit: colon goes on the next line, with 4 spaces of indentation relative to
> "NumericRelationTableElement(".

Done.

https://codereview.chromium.org/14046017/diff/23001/src/hydrogen-instructions...
src/hydrogen-instructions.h:880: NumericRelationTableElement(const
NumericRelationTableElement& other) :
On 2013/05/15 12:56:35, Jakob wrote:
> nit: same here

Done.