Runtime support for evaluation of static field initializer expressions

Runtime support for evaluation of static field initializer expressions

Eliminates the generation of field initializer functions by the parser.
Adds a runtime call that creates a one-shot function to evaluate the
initializer of a static field. The runtime function gets called from
the implicit static getter function for the field.

R=srdjan@google.com, zra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=39387

[hausner, 2014-08-14 23:31:26]

Srdjan, do you mind taking a first look while I work on the backend support for
the remaining platforms?

Thank you.

[Florian Schneider, 2014-08-15 09:56:35]

I like the simplification in the parser code.

I'd prefer a solution with only minimal platform-specific code for multiple
reasons. Not only because it is less code, but also e.g. in the current approach
the comparisons with sentinels can't be constant-folded away.

https://codereview.chromium.org/471283002/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/471283002/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language_ia32.cc:2458: LocationSummary*
InitStaticFieldInstr::MakeLocationSummary(Isolate* isolate,
I don't think we need a new instruction (and AST node) for this. It adds a lot
of platform-specific code duplication with no benefit.

Instead I'd just create the control flow AST in the parser:

temp = field.value;
if (temp === sentinel || temp === transition_sentinel) {
  call runtime
}

I think a generic CallRuntime instruction would be useful, since we already have
a couple of instructions that do exactly that.

[hausner, 2014-08-15 23:00:46]

This CL is complete now. Adding Zach as a reviewer to look at the mips and arm
platform code.

https://codereview.chromium.org/471283002/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/471283002/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language_ia32.cc:2458: LocationSummary*
InitStaticFieldInstr::MakeLocationSummary(Isolate* isolate,
On 2014/08/15 09:56:35, Florian Schneider wrote:

As Srdjan responded off-line, the optimizer can only eliminate the dead code for
const and final fields. We'd have to add special handling of comparison to
sentinel() and transition_sentinel() to handle non-final fields.

I agree that a solution with less platform-dependent code would be nicer, but it
would complicate unrelated instructions. The AST node and CallRuntimeInstruction
would still be necessary even if the guards were expressed in AST nodes and
corresponding backend nodes.

> I don't think we need a new instruction (and AST node) for this. It adds a lot
> of platform-specific code duplication with no benefit.
> 
> Instead I'd just create the control flow AST in the parser:
> 
> temp = field.value;
> if (temp === sentinel || temp === transition_sentinel) {
>   call runtime
> }
> 
> I think a generic CallRuntime instruction would be useful, since we already
have
> a couple of instructions that do exactly that.

[zra, 2014-08-18 15:56:09]

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:2643: locs->set_temp(0,
Location::RegisterLocation(R1));
Why does the temp register need to be hardcoded to R1? Same question for other
architectures.

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_mips.cc:2397: locs->set_out(0,
Location::RegisterLocation(T0));
If this should be the function result register for the result of the runtime
call, then use V0 here.

[zra, 2014-08-18 17:15:54]

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:2643: locs->set_temp(0,
Location::RegisterLocation(R1));
On 2014/08/18 15:56:09, zra wrote:
> Why does the temp register need to be hardcoded to R1? Same question for other
> architectures.

Nevermind. Looks like for kCall LocationSummary, you always have to hardcode the
registers.

[hausner, 2014-08-19 17:19:14]

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:2643: locs->set_temp(0,
Location::RegisterLocation(R1));
I initially tried to use Location::RequiresRegister() but that results in an
assertion, probably because of the reason you mention.

On 2014/08/18 17:15:53, zra wrote:
> On 2014/08/18 15:56:09, zra wrote:
> > Why does the temp register need to be hardcoded to R1? Same question for
other
> > architectures.
> 
> Nevermind. Looks like for kCall LocationSummary, you always have to hardcode
the
> registers.

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_mips.cc:2397: locs->set_out(0,
Location::RegisterLocation(T0));
On 2014/08/18 15:56:09, zra wrote:
> If this should be the function result register for the result of the runtime
> call, then use V0 here.

Done.

[zra, 2014-08-19 17:25:02]

arm/mips lgtm

[srdjan, 2014-08-19 18:49:39]

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:933: ASSERT(field.is_static());
Do you want to check if it was not already initialized and return then without
invoking the initializer?.

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:4641: virtual bool CanDeoptimize() const {
return true; }
Why can it deoptimize?

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:2642: locs->set_out(0,
Location::RegisterLocation(R0));
You do not need output .. here and elsewhere..

[hausner, 2014-08-19 20:54:00]

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:933: ASSERT(field.is_static());
On 2014/08/19 18:49:38, srdjan wrote:
> Do you want to check if it was not already initialized and return then without
> invoking the initializer?.

That check is done in compiled code. We only get here if the field is not yet
initialized. There's an assertion below to make sure this is the case.

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:4641: virtual bool CanDeoptimize() const {
return true; }
On 2014/08/19 18:49:39, srdjan wrote:
> Why can it deoptimize?

As discussed offline, the initializer expression can have side effects that
cause deoptimization.

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/471283002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:2642: locs->set_out(0,
Location::RegisterLocation(R0));
On 2014/08/19 18:49:39, srdjan wrote:
> You do not need output .. here and elsewhere..

Done.

[srdjan, 2014-08-19 21:04:51]

lgtm

[hausner, 2014-08-19 21:54:05]

Committed patchset #3 manually as r39387 (presubmit successful).