Add rotate-right instruction to hydrogen and use it instead of bitwise operations (issue 11033005)

[ul...@chromium.org]

Reviewers: danno, Michael Starzinger, Jay Conrod,

Message:
Please take a look.


https://chromiumcodereview.appspot.com/11033005/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/11033005/diff/1/src/hydrogen-instructions.h#newcode902
src/hydrogen-instructions.h:902: bool deleteable_operation =
IsBitwiseBinaryOperation();
danno, michael: any suggestions for other instruction types to add here?

https://chromiumcodereview.appspot.com/11033005/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/11033005/diff/1/src/hydrogen.cc#newcode3414
src/hydrogen.cc:3414: HPhase phase("H_EliminateUnusedInstructions",
this);
Maybe I should just do one pass instead of this complex worklist
algorithm?

Description:
Add rotate-right instruction to hydrogen and use it instead of bitwise
operations
of the form ((x >>> i) | (x << (32 - i))).

This CL is based on https://chromiumcodereview.appspot.com/10984057/
by Jay Conrod <dco...@codeaurora.org>.

R=da...@chromium.org,mstar...@chromium.org,dco...@codeaurora.org


Please review this at https://chromiumcodereview.appspot.com/11033005/

SVN Base: https://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.h
M src/hydrogen.cc
M src/ia32/assembler-ia32.h
M src/ia32/assembler-ia32.cc
M src/ia32/lithium-codegen-ia32.cc
M src/ia32/lithium-ia32.cc
M src/token.h
M src/x64/assembler-x64.h
M src/x64/lithium-codegen-x64.cc
M src/x64/lithium-x64.cc
A test/mjsunit/compiler/rotate.js

[dco...@codeaurora.org]

LGTM. I was worried having pattern matching inside the graph building phase
would overcomplicate things, but it actually seems simpler since you don't
have
to worry about representation or HChanges. I can't really comment on the
ia32/x64 parts. Does MIPS need support, too?

http://codereview.chromium.org/11033005/

[dco...@codeaurora.org]

lgtm




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

http://codereview.chromium.org/11033005/diff/2001/src/arm/simulator-arm.cc#newcode1503
src/arm/simulator-arm.cc:1503: } else {
Erik Corry expressed some suspicion in my first rev about the shifts,
and I agree. "result" is signed (int32_t), so >> will sign extend. Also
*carry_out should contain the last bit that was shifted, so it can be
(static_cast<uint32_t>(result) >> 31) != 0, right?

http://codereview.chromium.org/11033005/

[ul...@chromium.org]

I uploaded a new patch set that addresses the comments and fixes a
representation bug.

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

http://codereview.chromium.org/11033005/diff/2001/src/arm/simulator-arm.cc#newcode1503
src/arm/simulator-arm.cc:1503: } else {

On 2012/10/01 19:41:17, Jay Conrod wrote:
> Erik Corry expressed some suspicion in my first rev about the shifts,
and I
> agree. "result" is signed (int32_t), so >> will sign extend. Also
*carry_out
> should contain the last bit that was shifted, so it can be
> (static_cast<uint32_t>(result) >> 31) != 0, right?

Yes, you're right. I am taking the updated implementation that you
uploaded in the other CL.

http://codereview.chromium.org/11033005/