ARM64: Optimize generated code for gaps

ARM64: Optimize generated code for gaps

R=ulan@chromium.org, jochen@chromium.org

[vincent.belliard, 2014-05-01 13:53:30]

reduce the amount of instructions generated for gap resolution.
- use ldp/stp when two loads or two stores are contiguous
- use xzr if we need to store 0 into a stack slot.
- don't reload a constant in the scratch register if it's already loaded.
- use stp when the same constant must be stored in two contiguous locations.

The main goal is to reduce the number of generated instruction. The performances
are a little bit improved.

For Octane2, we need 80319 load/store to stack slots. 6132 are paired (3066
instructions saved). We also need to store 168 constants into stack slots. 148
are avoid (148 instructions saved).

[ulan, 2014-05-05 08:53:11]

I like the idea. I think we can make the implementation less fragile. 

If someone adds masm load/store to the gap resolver without checking for pending
operation, this might result in a hard to find bug.

It would be nice to have some kind of bottleneck between the gap resolver and
the masm. Maybe introduce an intermediate class that pretends to be a masm to
gap resolver, but has additional higher level methods like Store, StoreConstant,
Load, StackSlotMove, and correctly maintains the pending operation? WDYT?

[vincent.belliard, 2014-06-03 08:02:10]

Create a delayed macro assembler.

[ulan, 2014-06-04 09:28:20]

https://codereview.chromium.org/268673003/diff/40001/src/arm64/assembler-arm6...
File src/arm64/assembler-arm64.cc (right):

https://codereview.chromium.org/268673003/diff/40001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.cc:298: const MemOperand& operandA,
Nit: indent to four spaces

https://codereview.chromium.org/268673003/diff/40001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.cc:308: ((operandA.offset() & 0x7) != 0)) {
Is the offset required to be multiple of 8 or multiple of "access_size"?

Shoudn't the condition also check operandB.offset() if it comes before operandA?

https://codereview.chromium.org/268673003/diff/40001/src/arm64/assembler-arm64.h
File src/arm64/assembler-arm64.h (right):

https://codereview.chromium.org/268673003/diff/40001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.h:713: kPairBA };  // Can use a pair instruction
(operandB has lower address).
Nit: "};" should be in a separate line.

https://codereview.chromium.org/268673003/diff/40001/src/arm64/delayed-masm-a...
File src/arm64/delayed-masm-arm64.h (right):

https://codereview.chromium.org/268673003/diff/40001/src/arm64/delayed-masm-a...
src/arm64/delayed-masm-arm64.h:45: need_to_restore_root_ = true;
I think the root register and its management belong to GapResolver because it is
used both for breaking cycles and for storing constants. GapResolver knows that
it is correct because constant moves come after breaking cycles, but there is no
way to tell that by just looking at this class. So having root register in this
class introduces some implicit dependency between GapResolver and this class.

Can we instead pass the scratch register to StoreConstant and flush it in
EmitPending/ResetPending ?

[vincent.belliard, 2014-06-09 10:31:15]

https://codereview.chromium.org/268673003/diff/40001/src/arm64/assembler-arm6...
File src/arm64/assembler-arm64.cc (right):

https://codereview.chromium.org/268673003/diff/40001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.cc:298: const MemOperand& operandA,
On 2014/06/04 09:28:19, ulan wrote:
> Nit: indent to four spaces

Done.

https://codereview.chromium.org/268673003/diff/40001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.cc:308: ((operandA.offset() & 0x7) != 0)) {
Fixed: offset multiple of access size.

The following tests will check that abs(A.offset - B.offset) == access_size so,
if A.offset is multiple of access_size and this condition is true then B.offset
is multiple of access size.

https://codereview.chromium.org/268673003/diff/40001/src/arm64/assembler-arm64.h
File src/arm64/assembler-arm64.h (right):

https://codereview.chromium.org/268673003/diff/40001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.h:713: kPairBA };  // Can use a pair instruction
(operandB has lower address).
On 2014/06/04 09:28:19, ulan wrote:
> Nit: "};" should be in a separate line.

Done.

https://codereview.chromium.org/268673003/diff/40001/src/arm64/delayed-masm-a...
File src/arm64/delayed-masm-arm64.h (right):

https://codereview.chromium.org/268673003/diff/40001/src/arm64/delayed-masm-a...
src/arm64/delayed-masm-arm64.h:45: need_to_restore_root_ = true;
The scratch register is now defined by the caller (i.e. the new DelayedGapMasm
class). The new convention is that the scratch register is now used for
constants (StoreConstant). If the user of the DelayedMasm want to use the
scratch register, it must acquire/release it.
In debug, I added all the checks to ensure that the uses are consistent (for
example, that the gap resolver doesn't call StoreConstant between
acquire/release of the scratch register).

[ulan, 2014-06-10 08:54:40]

https://codereview.chromium.org/268673003/diff/60001/src/arm64/delayed-masm-a...
File src/arm64/delayed-masm-arm64.h (right):

https://codereview.chromium.org/268673003/diff/60001/src/arm64/delayed-masm-a...
src/arm64/delayed-masm-arm64.h:55: scratch_register_acquired_ = true;
Shouldn't we reset the saved value here since no longer control the scratch
register?

[vincent.belliard, 2014-06-10 09:00:12]

https://codereview.chromium.org/268673003/diff/60001/src/arm64/delayed-masm-a...
File src/arm64/delayed-masm-arm64.h (right):

https://codereview.chromium.org/268673003/diff/60001/src/arm64/delayed-masm-a...
src/arm64/delayed-masm-arm64.h:55: scratch_register_acquired_ = true;
This function is only used in debug to ensure that we don't use the scratch
register for a constant while the register is acquired.
If the scratch register is really used after acquired, the saved value will be
reset (or set to an other value).

[ulan, 2014-06-10 11:21:04]

On 2014/06/10 09:00:12, vincent.belliard.arm wrote:
>
https://codereview.chromium.org/268673003/diff/60001/src/arm64/delayed-masm-a...
> File src/arm64/delayed-masm-arm64.h (right):
> 
>
https://codereview.chromium.org/268673003/diff/60001/src/arm64/delayed-masm-a...
> src/arm64/delayed-masm-arm64.h:55: scratch_register_acquired_ = true;
> This function is only used in debug to ensure that we don't use the scratch
> register for a constant while the register is acquired.
> If the scratch register is really used after acquired, the saved value will be
> reset (or set to an other value).

I meant instead of checking for scratch register after each Ldr, Ldp, Mov,
wouldn't it be cleaner to reset the value to zero here once?

[vincent.belliard, 2014-06-12 12:57:57]

https://codereview.chromium.org/268673003/diff/60001/src/arm64/delayed-masm-a...
File src/arm64/delayed-masm-arm64.h (right):

https://codereview.chromium.org/268673003/diff/60001/src/arm64/delayed-masm-a...
src/arm64/delayed-masm-arm64.h:55: scratch_register_acquired_ = true;
On 2014/06/10 08:54:40, ulan wrote:
> Shouldn't we reset the saved value here since no longer control the scratch
> register? 

Done.

[ulan, 2014-06-14 07:57:21]

LGTM, thanks for fixing.