[Interpreter] Allocates new temporary register outside the reservation for consecutive registers.

[Interpreter] Allocates new temporary register outside the reservation for consecutive registers.

Consecutive registers are allocated in two passes. First we "reserve"
a set of registers and these get allocated when we actually use them.
If we request for a temporary register before we use all the consecutive
registers, the earlier implementation does not gaurantee that it allocates
outside the reservation for consecutive registers. This could cause problems
for example, in call_func(a, b++, c). This cl fixes
TemporaryRegisterScope::NewRegister, to return a new temporary register
outside the reservation for consecutive registers.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/e7373f42859cff71f2a64780dcab35244f1ce343
Cr-Commit-Position: refs/heads/master@{#33005}

[mythria, 2015-12-17 15:59:16]

mythria@chromium.org changed reviewers:
+ oth@chromium.org

[mythria, 2015-12-17 15:59:16]

This is to fix the following case: call(a, b++, c)
1. Call reserves 3 consecutive registers.
2. b++ requests for a register in outer scope, which uses up one of the
consecutive register.

Another fix is to make VisitCountOperation to always return value in
accumulator. I was not happy with that because it increases code size. I am not
very happy with this cl as well. It makes things more complex. Do let me know
any alternate suggestions. 

Thanks,
Mythri

[rmcilroy, 2015-12-17 18:53:01]

On 2015/12/17 15:59:16, mythria wrote:
> This is to fix the following case: call(a, b++, c)
> 1. Call reserves 3 consecutive registers.
> 2. b++ requests for a register in outer scope, which uses up one of the
> consecutive register.
> 
> Another fix is to make VisitCountOperation to always return value in
> accumulator. I was not happy with that because it increases code size. I am
not
> very happy with this cl as well. It makes things more complex. Do let me know
> any alternate suggestions. 

I much prefer this approach than modifying VisitCountOperation (I actually
thought the register allocator worked this way already). Does this really only
fix the tests in decodeURI or are there other test262 / mjsunit tests it fixes?

LGTM (but please wait for Orion's review before landing).

[oth, 2015-12-18 07:33:29]

Very nice! One comment, but a good fix.

lgtm

https://codereview.chromium.org/1531273002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1531273002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:995: for (; index !=
free_temporaries_.end(); index++) {
The code can save on the linear scan with:
Set<T>::llower_bound(start_index - 1)
Set<T>::upper_bound(end_index)

[oth, 2015-12-18 07:46:43]

https://codereview.chromium.org/1531273002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1531273002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:1424: allocated =
builder_->BorrowTemporaryRegisterNotInRange(
We might want to think about:

temporary_register_scope_A has partially fulfilled consecutive allocations.
temporary_register_scope_B nest under A has partially fulfilled consecutive
allocations.

Code allocates register in scope A. The allocator then ensures the allocation is
not taken from A's range, but B's consecutive range is now broken. 

The code could debug check no scopes under the scope being allocated in have
partially fulfilled consecutive ranges.  

Or when the scope allocates a consecutive range, we take all those registers out
of the free temporaries pool so they are not available for allocation.

[mythria, 2015-12-21 12:18:36]

Could you please take a look. I addressed comments and also changed the
implementation to handle the cases you pointed out.

https://codereview.chromium.org/1531273002/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1531273002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:995: for (; index !=
free_temporaries_.end(); index++) {
On 2015/12/18 07:33:29, oth wrote:
> The code can save on the linear scan with:
> Set<T>::llower_bound(start_index - 1)
> Set<T>::upper_bound(end_index)
> 

Done.

https://codereview.chromium.org/1531273002/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:1424: allocated =
builder_->BorrowTemporaryRegisterNotInRange(
On 2015/12/18 07:46:43, oth wrote:
> We might want to think about:
> 
> temporary_register_scope_A has partially fulfilled consecutive allocations.
> temporary_register_scope_B nest under A has partially fulfilled consecutive
> allocations.
> 
> Code allocates register in scope A. The allocator then ensures the allocation
is
> not taken from A's range, but B's consecutive range is now broken. 
> 
> The code could debug check no scopes under the scope being allocated in have
> partially fulfilled consecutive ranges.  
> 
> Or when the scope allocates a consecutive range, we take all those registers
out
> of the free temporaries pool so they are not available for allocation.

Thanks, I did not think of this case. I changed it to handle this case now. It
is not optimal. I just allocate a new unallocated temporary register whenever
there are more than one level of inner scopes or if the inner scope has a
consecutive allocation in progress. I hope it is not a frequent case, and hence
might be ok. This is handled EspressionResultScope::NewRegister

[mythria, 2015-12-21 12:23:43]

On 2015/12/17 18:53:01, rmcilroy wrote:
> On 2015/12/17 15:59:16, mythria wrote:
> > This is to fix the following case: call(a, b++, c)
> > 1. Call reserves 3 consecutive registers.
> > 2. b++ requests for a register in outer scope, which uses up one of the
> > consecutive register.
> > 
> > Another fix is to make VisitCountOperation to always return value in
> > accumulator. I was not happy with that because it increases code size. I am
> not
> > very happy with this cl as well. It makes things more complex. Do let me
know
> > any alternate suggestions. 
> 
> I much prefer this approach than modifying VisitCountOperation (I actually
> thought the register allocator worked this way already). Does this really only
> fix the tests in decodeURI or are there other test262 / mjsunit tests it
fixes?
> 
> LGTM (but please wait for Orion's review before landing).

I will check mjsunit. I did not check them. I thought we had a blacklist only
for test262.
For test262, there are few more tests(around 10-15) it fixes, but they are very
few and they are a part
of the tests which we skip with a wildcard entry. So if I remove them I have to
add a SKIP for 
a lot of tests, so I thought I will do it in a different cl. Some of them might
be fixed because
of the orion's cl that implements labeled break blocks.

[oth, 2015-12-21 12:48:21]

Like it - there are a couple of minor comments, but overall super. Will wait on
tests before final approval.

https://codereview.chromium.org/1531273002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1531273002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1002: if (index ==
free_temporaries_.end()) {
If this check only applies if upper_bound() has been called, the then block
could be moved directly after the upper_bound() call and the else block be run
unconditionally at the end of this function.

https://codereview.chromium.org/1531273002/diff/40001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1531273002/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:3097:
TEST(TemporaryRegiserAllocation) {
s/Regiser/Register/

[mythria, 2015-12-22 09:23:10]

finally got all the bots working :). I Updated mjsunit.status.

Thanks,
Mythri

[mythria, 2015-12-22 09:24:33]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-12-22 09:24:33]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1531273002/#ps120001
(title: "Fixing mjsunit.status again. one test is failing on bots.")

[commit-bot: I haz the power, 2015-12-22 09:24:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1531273002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1531273002/120001

[commit-bot: I haz the power, 2015-12-22 09:26:10]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-12-22 09:26:28]

Description was changed from

==========
[Interpreter] Allocates new temporary register outside the reservation for
consecutive registers.

Consecutive registers are allocated in two passes. First we "reserve"
a set of registers and these get allocated when we actually use them.
If we request for a temporary register before we use all the consecutive
registers, the earlier implementation does not gaurantee that it allocates
outside the reservation for consecutive registers. This could cause problems
for example, in call_func(a, b++, c). This cl fixes
TemporaryRegisterScope::NewRegister, to return a new temporary register
outside the reservation for consecutive registers.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Allocates new temporary register outside the reservation for
consecutive registers.

Consecutive registers are allocated in two passes. First we "reserve"
a set of registers and these get allocated when we actually use them.
If we request for a temporary register before we use all the consecutive
registers, the earlier implementation does not gaurantee that it allocates
outside the reservation for consecutive registers. This could cause problems
for example, in call_func(a, b++, c). This cl fixes
TemporaryRegisterScope::NewRegister, to return a new temporary register
outside the reservation for consecutive registers.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/e7373f42859cff71f2a64780dcab35244f1ce343
Cr-Commit-Position: refs/heads/master@{#33005}
==========

[commit-bot: I haz the power, 2015-12-22 09:26:29]

Patchset 7 (id:??) landed as
https://crrev.com/e7373f42859cff71f2a64780dcab35244f1ce343
Cr-Commit-Position: refs/heads/master@{#33005}