Make --always-opt also optimize toplevel code.

Make --always-opt also optimize toplevel code.

R=jacob.bramley@arm.com, titzer@chromium.org, rossberg@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=22666

[Michael Starzinger, 2014-07-23 13:20:46]

There are still two tests in cctest that I need to adapt and the architecture
ports are still missing. But I wanted to get a first round of review before I
polish it up.

[Michael Starzinger, 2014-07-28 09:01:49]

Adding Ben as reviewer.

[titzer, 2014-07-28 09:56:10]

lgtm other than small comments

https://codereview.chromium.org/410153002/diff/1/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/410153002/diff/1/src/factory.cc#newcode1376
src/factory.cc:1376: !isolate()->bootstrapper()->IsActive() &&
Comment me, TODO me?

https://codereview.chromium.org/410153002/diff/1/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/410153002/diff/1/test/cctest/test-heap.cc#new...
test/cctest/test-heap.cc:4039: if (i::FLAG_always_opt) return;
Can we get a TODO in here? It needs some thought on how to make it work with
more optimized functions, but the coverage is probably worth it.

https://codereview.chromium.org/410153002/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/410153002/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:2988: void MockUseCounterCallback(v8::Isolate*
isolate,
While you're here, can you rename this? It's not a mock.

https://codereview.chromium.org/410153002/diff/1/test/mjsunit/debug-compile-e...
File test/mjsunit/debug-compile-event-newfunction.js (right):

https://codereview.chromium.org/410153002/diff/1/test/mjsunit/debug-compile-e...
test/mjsunit/debug-compile-event-newfunction.js:63: // Create a function from
its body text. It will lead to an eval.
OK, just TODO someone

[Michael Starzinger, 2014-07-28 15:10:52]

Addressed comments. Ported to other architectures. Adapted two more test cases.
PTAL.

Rodolph, could you check if the change to the ARM64 is OK?

https://codereview.chromium.org/410153002/diff/1/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/410153002/diff/1/src/factory.cc#newcode1376
src/factory.cc:1376: !isolate()->bootstrapper()->IsActive() &&
On 2014/07/28 09:56:10, titzer wrote:
> Comment me, TODO me?

Done.

https://codereview.chromium.org/410153002/diff/1/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/410153002/diff/1/test/cctest/test-heap.cc#new...
test/cctest/test-heap.cc:4039: if (i::FLAG_always_opt) return;
On 2014/07/28 09:56:10, titzer wrote:
> Can we get a TODO in here? It needs some thought on how to make it work with
> more optimized functions, but the coverage is probably worth it.

Done.

https://codereview.chromium.org/410153002/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/410153002/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:2988: void MockUseCounterCallback(v8::Isolate*
isolate,
On 2014/07/28 09:56:10, titzer wrote:
> While you're here, can you rename this? It's not a mock.

Done.

https://codereview.chromium.org/410153002/diff/1/test/mjsunit/debug-compile-e...
File test/mjsunit/debug-compile-event-newfunction.js (right):

https://codereview.chromium.org/410153002/diff/1/test/mjsunit/debug-compile-e...
test/mjsunit/debug-compile-event-newfunction.js:63: // Create a function from
its body text. It will lead to an eval.
On 2014/07/28 09:56:10, titzer wrote:
> OK, just TODO someone

Done (put it one line below to avoid adapting line numbers).

https://codereview.chromium.org/410153002/diff/40001/src/arm64/lithium-codege...
File src/arm64/lithium-codegen-arm64.cc (left):

https://codereview.chromium.org/410153002/diff/40001/src/arm64/lithium-codege...
src/arm64/lithium-codegen-arm64.cc:5038:
ASM_UNIMPLEMENTED_BREAK("DoDeclareGlobals");
Rodolph, is this OK?

[jbramley, 2014-07-28 16:00:32]

Rodolph's not in the office today so he asked me to reply.

https://codereview.chromium.org/410153002/diff/80001/src/arm64/lithium-codege...
File src/arm64/lithium-codegen-arm64.cc (left):

https://codereview.chromium.org/410153002/diff/80001/src/arm64/lithium-codege...
src/arm64/lithium-codegen-arm64.cc:5038:
ASM_UNIMPLEMENTED_BREAK("DoDeclareGlobals");
We used ASM_UNIMPLEMENTED_BREAK during development. I thought they were all
gone, to be honest. This one probably remains because we couldn't test this
code. (That is, it's probably safe to remove it.)

https://codereview.chromium.org/410153002/diff/80001/src/arm64/lithium-codege...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/410153002/diff/80001/src/arm64/lithium-codege...
src/arm64/lithium-codegen-arm64.cc:694: __
Push(info()->scope()->GetScopeInfo());
I think it would be slightly better load the scope info explicity so we can
merge the pushes:

UseScratchRegisterScope temps(masm);
Register scope_info = temps.AcquireX();
__ Mov(scope_info, Operand(info()->scope()->GetScopeInfo()));
__ Push(x1, scope_info);

Each 'Push' has an overhead, and Push(Handle<ScopeInfo>) will just load the
handle into a scratch register anyway so the 'Mov' here doesn't have any extra
cost.

[Michael Starzinger, 2014-07-28 16:15:54]

Thanks. Addressed comments.

https://codereview.chromium.org/410153002/diff/80001/src/arm64/lithium-codege...
File src/arm64/lithium-codegen-arm64.cc (left):

https://codereview.chromium.org/410153002/diff/80001/src/arm64/lithium-codege...
src/arm64/lithium-codegen-arm64.cc:5038:
ASM_UNIMPLEMENTED_BREAK("DoDeclareGlobals");
On 2014/07/28 16:00:32, jbramley wrote:
> We used ASM_UNIMPLEMENTED_BREAK during development. I thought they were all
> gone, to be honest. This one probably remains because we couldn't test this
> code. (That is, it's probably safe to remove it.)

Acknowledged. This actually was the last use. Would you be fine if I removed the
macro (and the related flag) in a follow-up CL or do you intend to keep this
around?

https://codereview.chromium.org/410153002/diff/80001/src/arm64/lithium-codege...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/410153002/diff/80001/src/arm64/lithium-codege...
src/arm64/lithium-codegen-arm64.cc:694: __
Push(info()->scope()->GetScopeInfo());
On 2014/07/28 16:00:32, jbramley wrote:
> I think it would be slightly better load the scope info explicity so we can
> merge the pushes:
> 
> UseScratchRegisterScope temps(masm);
> Register scope_info = temps.AcquireX();
> __ Mov(scope_info, Operand(info()->scope()->GetScopeInfo()));
> __ Push(x1, scope_info);
> 
> Each 'Push' has an overhead, and Push(Handle<ScopeInfo>) will just load the
> handle into a scratch register anyway so the 'Mov' here doesn't have any extra
> cost.

Done.

[jbramley, 2014-07-28 16:28:11]

Thanks!

lgtm

https://codereview.chromium.org/410153002/diff/80001/src/arm64/lithium-codege...
File src/arm64/lithium-codegen-arm64.cc (left):

https://codereview.chromium.org/410153002/diff/80001/src/arm64/lithium-codege...
src/arm64/lithium-codegen-arm64.cc:5038:
ASM_UNIMPLEMENTED_BREAK("DoDeclareGlobals");
On 2014/07/28 16:15:53, Michael Starzinger wrote:
> On 2014/07/28 16:00:32, jbramley wrote:
> > We used ASM_UNIMPLEMENTED_BREAK during development. I thought they were all
> > gone, to be honest. This one probably remains because we couldn't test this
> > code. (That is, it's probably safe to remove it.)
> 
> Acknowledged. This actually was the last use. Would you be fine if I removed
the
> macro (and the related flag) in a follow-up CL or do you intend to keep this
> around?

I see no reason to keep it. We can recreate it fairly easily if we ever find
that we need it again, but it seems unlikely.

[Michael Starzinger, 2014-07-29 11:41:54]

Committed patchset #6 manually as r22666 (presubmit successful).