Reviewers: Erik Corry, ulan, Michael Starzinger, danno,
Message:
On 2012/09/27 23:53:27, Michael Starzinger wrote:
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)
In this case, we focused on getting StringSHA1 performance to improve
quickly. I
agree that a more general approach would be best. For this kind of thing, a
tiling instruction selector would work very well, but it's hard to implement
that kind of thing with the coarse granularity of Lithium. Perhaps we can
work
together on something of this nature in the future. It would definitely
help ARM
platforms which are more sensitive to instructions emitted than x86.
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.
Rotations are common in cryptography, and pretty much every major benchmark
has
a crypto component. When we wrote this, we were targeting StringSHA1 in
BrowserMark, which has the following function:
rotateLeft: function(n, s) {
var t4 = ( n << s ) | (n >>> (32 - s));
return t4;
},
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.
I think it's strange that the LShift instruction uses AST tokens to
determine
what operation to perform. It seems like a common pattern, so we didn't
want to
change it. Maybe it would be better to have separate enums for syntax and
operations, since, as in this case, we want to represent a wider variety of
operations than the syntax allows.
4) This change definitely needs better test coverage.
Agree.
I have refactored this change, incorporating as much as the above feedback
as I
could (although I'm not making any architectural changes for now). Once the
new
revision passes legal review (should be Monday), I'll upload here. Test
cases
will follow soon after that.
Description:
Replace a set of Hydrogen instructions with rotate instructions on ARM
BUG=none
TEST=none
Please review this at http://codereview.chromium.org/10984057/
SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/
Affected files:
M src/arm/lithium-arm.cc
M src/arm/lithium-codegen-arm.cc
M src/arm/simulator-arm.cc
M src/hydrogen-instructions.h
M src/hydrogen-instructions.cc
M src/hydrogen.h
M src/hydrogen.cc
M src/ia32/lithium-ia32.cc
M src/token.h
M src/x64/lithium-x64.cc
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
