ARM64: Use the shifter operand to merge in previous shift instructions.

ARM64: Use the shifter operand to merge in previous shift instructions.

When possible, we transform sequences of code of the form
    lsl x8, x9, #imm
    add x0, x1, x8
into
    add x0, x1, x9 LSL #imm

[ulan, 2014-04-29 15:01:44]

Alexandre, first question before reviewing: does this optimization trigger in
benchmarks? If so how does it affect performance?

[Alexandre Rames, 2014-04-29 15:13:15]

On 2014/04/29 15:01:44, ulan wrote:
> Alexandre, first question before reviewing: does this optimization trigger in
> benchmarks? If so how does it affect performance?

The patch gives a ~15% performance boost on v8/Crypto.

[ulan, 2014-05-02 10:01:18]

Although it adds a lot of complexity, I think 15% performance boost can justify
landing it.

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

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.cc#n...
src/arm64/lithium-arm64.cc:2047: if (!val->IsBitwise() && !(val->IsAdd() ||
val->IsSub())) return NULL;
Simpler condition: !(val->IsBitwise() || val->IsAdd() || val->IsSub())

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.cc#n...
src/arm64/lithium-arm64.cc:2139: bool can_overflow =
hinstr->CheckFlag(HValue::kCanOverflow);
You can inline hinstr->CheckFlag(HValue::kCanOverflow) below without
can_overflow

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.cc#n...
src/arm64/lithium-arm64.cc:2148: UNREACHABLE();
nit: I'd ASSERT(hinstr->IsSub()) above.

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.h
File src/arm64/lithium-arm64.h (right):

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.h#ne...
src/arm64/lithium-arm64.h:563: public:
Google style guide doesn't allow non-pure multiple inheritance.

One possible solution is to turn each function that accepts
LShiftedRightOpInterface* into a template with L-instructions as a template
parameter.

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.h#ne...
src/arm64/lithium-arm64.h:3098: return Assembler::IsImmAddSub(imm) ||
Assembler::IsImmAddSub(-imm);
Did you mean && instead of ||?

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.h#ne...
src/arm64/lithium-arm64.h:3104: UNREACHABLE();
nit: I'd slightly prefer a more compact ASSERT(instr->IsBitwise()) above without
this branch.

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-codegen-ar...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-codegen-ar...
src/arm64/lithium-codegen-arm64.cc:1268:
ToInteger32(LConstantOperand::cast(shift_info->shift_amount())) & 0x1f);
Could you name the 0x1f constant since it is used in other places too?

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-codegen-ar...
src/arm64/lithium-codegen-arm64.cc:1270: }
Two empty lines after function.

[Alexandre Rames, 2014-05-02 13:51:44]

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

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.cc#n...
src/arm64/lithium-arm64.cc:2047: if (!val->IsBitwise() && !(val->IsAdd() ||
val->IsSub())) return NULL;
On 2014/05/02 10:01:18, ulan wrote:
> Simpler condition: !(val->IsBitwise() || val->IsAdd() || val->IsSub())

Done.

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.cc#n...
src/arm64/lithium-arm64.cc:2139: bool can_overflow =
hinstr->CheckFlag(HValue::kCanOverflow);
On 2014/05/02 10:01:18, ulan wrote:
> You can inline hinstr->CheckFlag(HValue::kCanOverflow) below without
> can_overflow

Done.

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.cc#n...
src/arm64/lithium-arm64.cc:2148: UNREACHABLE();
On 2014/05/02 10:01:18, ulan wrote:
> nit: I'd ASSERT(hinstr->IsSub()) above.

Done.

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.h
File src/arm64/lithium-arm64.h (right):

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.h#ne...
src/arm64/lithium-arm64.h:563: public:
On 2014/05/02 10:01:18, ulan wrote:
> Google style guide doesn't allow non-pure multiple inheritance.
> 
> One possible solution is to turn each function that accepts
> LShiftedRightOpInterface* into a template with L-instructions as a template
> parameter.

Done.

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.h#ne...
src/arm64/lithium-arm64.h:3098: return Assembler::IsImmAddSub(imm) ||
Assembler::IsImmAddSub(-imm);
On 2014/05/02 10:01:18, ulan wrote:
> Did you mean && instead of ||?

'||' was intended. IsImmAddSub() works with unsigned values.

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-arm64.h#ne...
src/arm64/lithium-arm64.h:3104: UNREACHABLE();
On 2014/05/02 10:01:18, ulan wrote:
> nit: I'd slightly prefer a more compact ASSERT(instr->IsBitwise()) above
without
> this branch.

Done.

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-codegen-ar...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-codegen-ar...
src/arm64/lithium-codegen-arm64.cc:1268:
ToInteger32(LConstantOperand::cast(shift_info->shift_amount())) & 0x1f);
On 2014/05/02 10:01:18, ulan wrote:
> Could you name the 0x1f constant since it is used in other places too?

Abstracted the constant and used a helper.

https://codereview.chromium.org/257203002/diff/1/src/arm64/lithium-codegen-ar...
src/arm64/lithium-codegen-arm64.cc:1270: }
On 2014/05/02 10:01:18, ulan wrote:
> Two empty lines after function.

Done.

[Alexandre Rames, 2014-05-02 15:48:53]

Uploaded the updated patch.

[ulan, 2014-05-05 07:30:45]

lgtm

[Alexandre Rames, 2014-05-19 08:03:07]

On 2014/05/05 07:30:45, ulan wrote:
> lgtm

Was landed by Martyn in r21161.