Replace a set of Hydrogen instructions with rotate instructions on ARM (issue 10984057)

[erik....@gmail.com]

http://codereview.chromium.org/10984057/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/10984057/diff/1/src/arm/simulator-arm.cc#newcode1504
src/arm/simulator-arm.cc:1504: int32_t left = result >> shift_amount;
How does this work with a negative input?

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

http://codereview.chromium.org/10984057/diff/1/src/hydrogen-instructions.cc#newcode1624
src/hydrogen-instructions.cc:1624: : new(zone) Range();
Don't you have to Ror the result here?

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

http://codereview.chromium.org/10984057/diff/1/src/hydrogen-instructions.h#newcode3638
src/hydrogen-instructions.h:3638:
ChangeRepresentation(Representation::Integer32());
Why is HRor different from all other bit ops here?

http://codereview.chromium.org/10984057/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10984057/diff/1/src/hydrogen.cc#newcode1043
src/hydrogen.cc:1043: !shl->representation().IsInteger32())
Missing {}

http://codereview.chromium.org/10984057/diff/1/src/hydrogen.cc#newcode1052
src/hydrogen.cc:1052: if (!shr->right()->IsSub()) return false;
This function needs a comment explaining what it is looking for, and the
line above here needs an explanation of why only a sub function is OK
here.

http://codereview.chromium.org/10984057/diff/1/src/hydrogen.cc#newcode1062
src/hydrogen.cc:1062: if (sub->right()->OperandAt(0) !=
shl->right()->OperandAt(0))
Missing {}

http://codereview.chromium.org/10984057/diff/1/src/hydrogen.cc#newcode1083
src/hydrogen.cc:1083: bool HGraph::TryRotateRight(HInstruction* instr) {
There is a lot of copied code here - can they be combined into one
function with a flag?

http://codereview.chromium.org/10984057/diff/1/src/token.h
File src/token.h (right):

http://codereview.chromium.org/10984057/diff/1/src/token.h#newcode102
src/token.h:102: T(ROR, "rotate right", 11)
\
This needs a comment to explain that it is only used in Crankshaft.

http://codereview.chromium.org/10984057/

[ul...@chromium.org]

http://codereview.chromium.org/10984057/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/10984057/diff/1/src/arm/simulator-arm.cc#newcode1507
src/arm/simulator-arm.cc:1507: *carry_out = false;
The carry_out should contain the last shifted bit according to the spec.

http://codereview.chromium.org/10984057/diff/1/src/arm/simulator-arm.cc#newcode1511
src/arm/simulator-arm.cc:1511:
We can remove this empty line.

http://codereview.chromium.org/10984057/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10984057/diff/1/src/hydrogen.cc#newcode1041
src/hydrogen.cc:1041: if (shl->UseCount()>1 || shr->UseCount()>1) return
false;
Please add space before and after each ">" operator.

http://codereview.chromium.org/10984057/diff/1/src/hydrogen.cc#newcode1089
src/hydrogen.cc:1089: if (bor->left()->IsShl() && bor->right()->IsShr())
{
Please add space before and after each ">" operator.

http://codereview.chromium.org/10984057/

[ul...@chromium.org]

http://codereview.chromium.org/10984057/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10984057/diff/1/src/hydrogen.cc#newcode3539
src/hydrogen.cc:3539: #if defined(V8_TARGET_ARCH_ARM)
I am concerned with this ifdef. Hydrogen should contain platform
independent code and these kind of optimizations should be done on the
Lithium level.

http://codereview.chromium.org/10984057/

[mstar...@chromium.org]

Some general high-level observations about this change:

1) This is a very specialized optimization targeting one specific
instruction
(i.e. rotation). I think a more general approach that performs general
pattern
matching might scale better and cover other peephole optimizations as well
(e.g.
scaled array access, algebraic simplification or other strength reductions)

2) Is this a common pattern in JavaScript applications or is this tracked
by any
benchmark? I would be interested to know what particular use case this
optimization targets.

3) Adding a token for something that has no equivalent in JavaScript seems
wrong. We only do this in rare cases where we need to push information about
variable initialization through the AST, but not for Crankshaft
optimizations.

4) This change definitely needs better test coverage.

https://chromiumcodereview.appspot.com/10984057/