Don't optimize away static field initialization when precompiling.

Don't optimize away static field initialization when precompiling.

R=hausner@google.com

Committed: https://github.com/dart-lang/sdk/commit/718ae3a70495f41251f45d52ea70b3ec26ec3362

[rmacnak, 2015-09-10 20:53:39]

rmacnak@google.com changed reviewers:
+ hausner@google.com

[hausner, 2015-09-10 22:55:12]

LGTM but I wish that we could avoid another specialized flag, and I would love
to see a comment that explains the need for the flag.

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

https://codereview.chromium.org/1327383002/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:1311: if (FLAG_disassemble) {
I think you should add the disassemble filter like in other places:
FlowGraphPrinter::ShouldPrint(...)

Actually, isn't the function already disassembled by
CompileParsedFunctionHelper?

https://codereview.chromium.org/1327383002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/1327383002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:41: "Don't optimize away static field
initialization");
The flag name and the description are quite different. It would be nice to have
a comment explaining what is going on. (Why can't the initializers be optimized
away?)

OTOH, do we really need a separate flag for this? I'm not familiar with all the
different flags we have that affect the VM when pre-compilation is used, but
maybe this can be combined with one of the other flags.

[rmacnak, 2015-09-11 00:07:13]

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

https://codereview.chromium.org/1327383002/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:1311: if (FLAG_disassemble) {
On 2015/09/10 22:55:12, hausner wrote:
> I think you should add the disassemble filter like in other places:
> FlowGraphPrinter::ShouldPrint(...)
> 
> Actually, isn't the function already disassembled by
> CompileParsedFunctionHelper?

Hm, yes. I guess I was missing them from my disassembly for other reasons.

https://codereview.chromium.org/1327383002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/1327383002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:41: "Don't optimize away static field
initialization");
On 2015/09/10 22:55:12, hausner wrote:
> The flag name and the description are quite different. It would be nice to
have
> a comment explaining what is going on. (Why can't the initializers be
optimized
> away?)

Added:

  // When precompiling, the fact that a field is currently initialized does not
  // make it safe to omit code that checks if the field needs initialization
  // because the field will be reset so it starts uninitialized in the process
  // running the precompiled code. We must be prepared to reinitialize fields.

> OTOH, do we really need a separate flag for this? I'm not familiar with all
the
> different flags we have that affect the VM when pre-compilation is used, but
> maybe this can be combined with one of the other flags.


--precompile is a flag handler that flips a number of other flags. Having
separate flags lets us test alternate compilations independent of
precompilation.

[rmacnak, 2015-09-11 00:27:43]

Committed patchset #2 (id:20001) manually as
718ae3a70495f41251f45d52ea70b3ec26ec3362 (presubmit successful).