Infrastructure classes for evaluating numeric relations between values.

Infrastructure classes for evaluating numeric relations between values.

Committed: https://code.google.com/p/v8/source/detail?r=13656

[Massi, 2013-02-12 13:04:03]

This series of patches is the current code I have for array bounds check
elimination.
It is possible to commit each patch separately (in sequence) but I have put all
of them here for simplicity.

[Jakob Kummerow, 2013-02-12 15:10:42]

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

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:693: HValue*
HValue::AddNumericConstraint(HInstruction* insertion_point,
I don't think we need this. Let's just inline it everywhere.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:805: HNumericConstraint*
HNumericConstraint::New(HInstruction* insertion_point,
This should probably be renamed (AddToGraph?). Also, please reorder the
arguments: constrained_value, relation, related_value, insertion_point.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:848: HValue* related_value) {
nit: indentation

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:850: return
NumericRelation::Lt().Implies(relation);
How about a comment:
// We know that a BoundsCheck is smaller than the length it compared against.

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

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:843: class NumericRelation {
I'd prefer making this a top-level class.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:859: const char* Mnemonic() const { return
MnemonicFromKind((Kind) kind_); }
C++ style casts please

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:861: static NumericRelation None() { return
NumericRelation(NONE); }
I don't think these add much value. Let's just call the constructor.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:869: bool operator==(const NumericRelation& other) {
I'd prefer an Equals() method over operator overloading. (If these are used at
all.)

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:909: default:
no default case

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1003: virtual bool CheckRelation(NumericRelation
relation, HValue* other) {
This method's name should clearly express its relation to IsRelationTrue().
Maybe IsRelationTrueInternal? I'm open to better suggestions.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1245: return
constrained_value()->RequiredInputRepresentation(index);
return representation() (or return constrained_value->representation(), which is
the same thing).

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1262: explicit HNumericConstraint(HValue*
constrained_value,
no "explicit"

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1263: HValue* related_value,
please swap the last two arguments

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1265: : relation_(relation) {
nit: 4-space indent

https://codereview.chromium.org/12226112/diff/1/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/12226112/diff/1/src/ia32/lithium-ia32.cc#newc...
src/ia32/lithium-ia32.cc:1716: return NULL;
Consider marking this as UNREACHABLE().

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

https://codereview.chromium.org/12226112/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:533: ASSERT(IsInteger32Constant());
we don't need this ASSERT, as the cast and Integer32Value() below contain the
same checks.

https://codereview.chromium.org/12226112/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1943: bool HShr::CheckRelation(NumericRelation
relation, HValue* other) {
Let's skip these too for now.

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

https://codereview.chromium.org/12226112/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:919: // "rel.IsExtendable(direction) is true" then
nit: position of the closing quote:
    // "rel.IsExtendable(direction)" is true then

https://codereview.chromium.org/12226112/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3963: return relation.IsExtendable(offset)
-offset

https://codereview.chromium.org/12226112/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3998: // The idea is that "k > 1" implies that "base
* k >= base".
As discussed, we should probably postpone this implementation until we have a
reason to add it :-)

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

https://codereview.chromium.org/12226112/diff/4001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1534: if
(OperandAt(0)->IsRelationTrue(NumericRelation::Ge(), this)) {
I don't feel strongly about it, but you *could* avoid spelling out all these
cases explicitly:

static const NumericRelation[] relations = { NumericRelation::Ge(),
NumericRelation::Le() };
for (rel_i = 0; rel_i < 2; rel_i++) {
  NumericRelation rel = relations[rel_i];
  for (i = 0; i <= 1; i++) {
    if (Operand(i)->IsRelationTrue(relation, this)) {
      induction_base = OperandAt((i+1) % 2);
      relation = rel;
    }
  }
}

https://codereview.chromium.org/12226112/diff/4001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1539: relation = NumericRelation::Ge();
s/Ge/Le/

https://codereview.chromium.org/12226112/diff/4001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1545: relation = NumericRelation::Ge();
s/Ge/Le/

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

https://codereview.chromium.org/12226112/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:2054:
left()->AddNumericConstraint(SuccessorAt(0)->first(), right(), r.Reversed());
s/.Reversed()/

https://codereview.chromium.org/12226112/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:2055:
left()->AddNumericConstraint(SuccessorAt(1)->first(), right(), r);
and here you need r.Negated(), which is notably not the same as .Reversed()!

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

https://codereview.chromium.org/12226112/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.h:876: case Token::LT: return Gt();
Really? This is surprising.

https://codereview.chromium.org/12226112/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.h:880: case Token::NE: return Ne();
You may also want to support NE_STRICT

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

https://codereview.chromium.org/12226112/diff/11001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:859: void HBoundsCheck::AddInformativeDefinitions()
{
As discussed, having this logic in this pass is a hack, but good enough for now.

https://codereview.chromium.org/12226112/diff/11001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:861: NumericRelation::Ge(),
block()->graph()->GetConstant0())
indentation:

void HBoundsCheck::AddInformativeDefinitions() {
  if (index()->IsRelationTrue(NumericRelation::Ge(),
                              block()->graph()->GetConstant0()) &&
      index()->IsRelationTrue(NumericRelation::Lt(), length())) {
    set_skip_check(true);
  }
}

https://codereview.chromium.org/12226112/diff/11001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:863: set_skip_check(true);
As discussed, it should be possible to just remove the bounds check instead of
disabling it. But it's fine to leave it like this for now.

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

https://codereview.chromium.org/12226112/diff/10010/src/hydrogen-instructions...
src/hydrogen-instructions.cc:1546: if (OperandAt(0)->block()->is_osr_entry()) {
Let's fold this into the loop-based refactoring of the rest of the method:

NumericRelation[] relations = { NumericRelation::Ge(); NumericRelation::Le() };
for (i = 0; i <= 1; i++) {
  // Discard OSR entry point.
  if (OperandAt(i)->block()->is_osr_entry()) {
    ...
  }

  // Check if this phi is an induction variable.
  for (rel_i = 0; rel_i < 2; rel_i++) {
    NumericRelation rel = relations[rel_i];
    if (Operand(i)->IsRelationTrue(relation, this)) {
      induction_base = OperandAt((i+1) % 2);
      relation = rel;
    }
  }
}

[Massi, 2013-02-13 11:56:42]

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

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:693: HValue*
HValue::AddNumericConstraint(HInstruction* insertion_point,
On 2013/02/12 15:10:42, Jakob wrote:
> I don't think we need this. Let's just inline it everywhere.

Done.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:805: HNumericConstraint*
HNumericConstraint::New(HInstruction* insertion_point,
On 2013/02/12 15:10:42, Jakob wrote:
> This should probably be renamed (AddToGraph?). Also, please reorder the
> arguments: constrained_value, relation, related_value, insertion_point.

Done.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:848: HValue* related_value) {
On 2013/02/12 15:10:42, Jakob wrote:
> nit: indentation

Done.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:850: return
NumericRelation::Lt().Implies(relation);
On 2013/02/12 15:10:42, Jakob wrote:
> How about a comment:
> // We know that a BoundsCheck is smaller than the length it compared against.

Done.

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

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:843: class NumericRelation {
On 2013/02/12 15:10:42, Jakob wrote:
> I'd prefer making this a top-level class.

Done.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:859: const char* Mnemonic() const { return
MnemonicFromKind((Kind) kind_); }
On 2013/02/12 15:10:42, Jakob wrote:
> C++ style casts please

Done.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:861: static NumericRelation None() { return
NumericRelation(NONE); }
On 2013/02/12 15:10:42, Jakob wrote:
> I don't think these add much value. Let's just call the constructor.

Not having then would mean having to write constructor calls like
"NumericRelation(NumericRelation::EQ)", so these static methods make the code
less verbose in the end.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:869: bool operator==(const NumericRelation& other) {
On 2013/02/12 15:10:42, Jakob wrote:
> I'd prefer an Equals() method over operator overloading. (If these are used at
> all.)

I added an "IsNone()" method instead.
I think that in all other cases calling "Implies()" is better than checking for
"hard equality" anyway.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:909: default:
On 2013/02/12 15:10:42, Jakob wrote:
> no default case

Done.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1003: virtual bool CheckRelation(NumericRelation
relation, HValue* other) {
On 2013/02/12 15:10:42, Jakob wrote:
> This method's name should clearly express its relation to IsRelationTrue().
> Maybe IsRelationTrueInternal? I'm open to better suggestions.

Done.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1245: return
constrained_value()->RequiredInputRepresentation(index);
On 2013/02/12 15:10:42, Jakob wrote:
> return representation() (or return constrained_value->representation(), which
is
> the same thing).

Done.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1262: explicit HNumericConstraint(HValue*
constrained_value,
On 2013/02/12 15:10:42, Jakob wrote:
> no "explicit"

Done.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1263: HValue* related_value,
On 2013/02/12 15:10:42, Jakob wrote:
> please swap the last two arguments

Done.

https://codereview.chromium.org/12226112/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1265: : relation_(relation) {
On 2013/02/12 15:10:42, Jakob wrote:
> nit: 4-space indent

Done.

https://codereview.chromium.org/12226112/diff/1/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/12226112/diff/1/src/ia32/lithium-ia32.cc#newc...
src/ia32/lithium-ia32.cc:1716: return NULL;
On 2013/02/12 15:10:42, Jakob wrote:
> Consider marking this as UNREACHABLE().

Done.

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

https://codereview.chromium.org/12226112/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:533: ASSERT(IsInteger32Constant());
On 2013/02/12 15:10:42, Jakob wrote:
> we don't need this ASSERT, as the cast and Integer32Value() below contain the
> same checks.

Done.

https://codereview.chromium.org/12226112/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1943: bool HShr::CheckRelation(NumericRelation
relation, HValue* other) {
On 2013/02/12 15:10:42, Jakob wrote:
> Let's skip these too for now.

Done.

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

https://codereview.chromium.org/12226112/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:919: // "rel.IsExtendable(direction) is true" then
On 2013/02/12 15:10:42, Jakob wrote:
> nit: position of the closing quote:
>     // "rel.IsExtendable(direction)" is true then

Done.

https://codereview.chromium.org/12226112/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3963: return relation.IsExtendable(offset)
On 2013/02/12 15:10:42, Jakob wrote:
> -offset

Done.

https://codereview.chromium.org/12226112/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:3998: // The idea is that "k > 1" implies that "base
* k >= base".
On 2013/02/12 15:10:42, Jakob wrote:
> As discussed, we should probably postpone this implementation until we have a
> reason to add it :-)

Done.

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

https://codereview.chromium.org/12226112/diff/4001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1534: if
(OperandAt(0)->IsRelationTrue(NumericRelation::Ge(), this)) {
On 2013/02/12 15:10:42, Jakob wrote:
> I don't feel strongly about it, but you *could* avoid spelling out all these
> cases explicitly:
> 
> static const NumericRelation[] relations = { NumericRelation::Ge(),
> NumericRelation::Le() };
> for (rel_i = 0; rel_i < 2; rel_i++) {
>   NumericRelation rel = relations[rel_i];
>   for (i = 0; i <= 1; i++) {
>     if (Operand(i)->IsRelationTrue(relation, this)) {
>       induction_base = OperandAt((i+1) % 2);
>       relation = rel;
>     }
>   }
> }

Done.

https://codereview.chromium.org/12226112/diff/4001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1539: relation = NumericRelation::Ge();
On 2013/02/12 15:10:42, Jakob wrote:
> s/Ge/Le/

Done.

https://codereview.chromium.org/12226112/diff/4001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1545: relation = NumericRelation::Ge();
On 2013/02/12 15:10:42, Jakob wrote:
> s/Ge/Le/

Done.

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

https://codereview.chromium.org/12226112/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:2054:
left()->AddNumericConstraint(SuccessorAt(0)->first(), right(), r.Reversed());
On 2013/02/12 15:10:42, Jakob wrote:
> s/.Reversed()/

Done.

https://codereview.chromium.org/12226112/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:2055:
left()->AddNumericConstraint(SuccessorAt(1)->first(), right(), r);
On 2013/02/12 15:10:42, Jakob wrote:
> and here you need r.Negated(), which is notably not the same as .Reversed()!

Done.

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

https://codereview.chromium.org/12226112/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.h:876: case Token::LT: return Gt();
On 2013/02/12 15:10:42, Jakob wrote:
> Really? This is surprising.

Done.

https://codereview.chromium.org/12226112/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.h:880: case Token::NE: return Ne();
On 2013/02/12 15:10:42, Jakob wrote:
> You may also want to support NE_STRICT

Done.

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

https://codereview.chromium.org/12226112/diff/11001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:859: void HBoundsCheck::AddInformativeDefinitions()
{
On 2013/02/12 15:10:42, Jakob wrote:
> As discussed, having this logic in this pass is a hack, but good enough for
now.

Done.

https://codereview.chromium.org/12226112/diff/11001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:861: NumericRelation::Ge(),
block()->graph()->GetConstant0())
On 2013/02/12 15:10:42, Jakob wrote:
> indentation:
> 
> void HBoundsCheck::AddInformativeDefinitions() {
>   if (index()->IsRelationTrue(NumericRelation::Ge(),
>                               block()->graph()->GetConstant0()) &&
>       index()->IsRelationTrue(NumericRelation::Lt(), length())) {
>     set_skip_check(true);
>   }
> }

Done.

https://codereview.chromium.org/12226112/diff/11001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:863: set_skip_check(true);
On 2013/02/12 15:10:42, Jakob wrote:
> As discussed, it should be possible to just remove the bounds check instead of
> disabling it. But it's fine to leave it like this for now.

Done.

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

https://codereview.chromium.org/12226112/diff/10010/src/hydrogen-instructions...
src/hydrogen-instructions.cc:1546: if (OperandAt(0)->block()->is_osr_entry()) {
On 2013/02/12 15:10:42, Jakob wrote:
> Let's fold this into the loop-based refactoring of the rest of the method:
> 
> NumericRelation[] relations = { NumericRelation::Ge(); NumericRelation::Le()
};
> for (i = 0; i <= 1; i++) {
>   // Discard OSR entry point.
>   if (OperandAt(i)->block()->is_osr_entry()) {
>     ...
>   }
> 
>   // Check if this phi is an induction variable.
>   for (rel_i = 0; rel_i < 2; rel_i++) {
>     NumericRelation rel = relations[rel_i];
>     if (Operand(i)->IsRelationTrue(relation, this)) {
>       induction_base = OperandAt((i+1) % 2);
>       relation = rel;
>     }
>   }
> }

Done.

[Jakob Kummerow, 2013-02-13 13:20:56]

LGTM with a few more minor comments.

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

https://codereview.chromium.org/12226112/diff/17001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:860: void HBoundsCheck::AddInformativeDefinitions()
{
Please add a comment here:
  // TODO(mmassi): Executing this code during AddInformativeDefinitions
  // is a hack. Move it to some other HPhase.

https://codereview.chromium.org/12226112/diff/17001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:1554: this, NumericRelation::Eq(),
OperandAt(other_operand_index));
nit: 4 spaces indentation is enough

https://codereview.chromium.org/12226112/diff/17001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:1562: // Check if this phi is an induction
variable.
Please extend this comment to convey the reasoning behind the code:
      // Check if this phi is an induction variable. If, e.g., we know that
      // its first input is greater than the phi itself, then that must be
      // the back edge, and the phi is always greater than its second input.

https://codereview.chromium.org/12226112/diff/17001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:1989: 
nit: unneeded whitespace

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

https://codereview.chromium.org/12226112/diff/17001/src/hydrogen-instructions...
src/hydrogen-instructions.h:587: case Token::LT: return Gt();
Looks like you forgot to fix these two (LT, GT).

https://codereview.chromium.org/12226112/diff/17001/src/hydrogen-instructions...
src/hydrogen-instructions.h:619: case GT: return Ge();
return Le();

https://codereview.chromium.org/12226112/diff/17001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/12226112/diff/17001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:1716: UNREACHABLE();
In light of our most recent discussions, we are in fact still reaching this
code. Let's remove the UNREACHABLE for now, and deal with this later.

[Jakob Kummerow, 2013-02-13 13:54:54]

Almost...

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

https://codereview.chromium.org/12226112/diff/25001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:1993: 
Please remove one of these three empty lines.

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

https://codereview.chromium.org/12226112/diff/25001/src/hydrogen-instructions...
src/hydrogen-instructions.h:619: case GT: return Ge();
Still not fixed: return Le();

[Jakob Kummerow, 2013-02-13 14:01:44]

lgtm