Use Variable::binding_needs_init() to determine hole initialization

Use Variable::binding_needs_init() to determine hole initialization

The old code was using VariableMode, but that signal is both
over-pessimistic (some CONST and LET variables need no hole-initialization)
and inconsistent with other uses of the InitializationFlag enum (such
as %LoadLookupSlot).

This changes no observable behavior, but removes unnecessary hole
initialization and hole checks in a few places, including
block-scoped function declarations, super property lookups,
and new.target.

R=bmeurer@chromium.org, neis@chromium.org

Committed: https://crrev.com/6768456db513936cb19c8cdc657a7cd6122584a9
Cr-Commit-Position: refs/heads/master@{#38395}

[adamk, 2016-08-03 22:02:25]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 22:02:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 22:07:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-03 22:07:44]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)

[adamk, 2016-08-03 22:09:40]

I think this is ready for review, but I'm also working on a followup that
switches more places where backends are checking VariableMode to check
binding_needs_init instead.

[adamk, 2016-08-03 22:18:34]

Description was changed from

==========
Use Variable::binding_needs_init() to determine hole initialization

The old code was using VariableMode, but that signal is both
over-pessimistic (some CONST and LET variables need no hole-initialization)
and inconsistent with other uses of the InitializationFlag enum (such
as %LoadLookupSlot).

This changes no observable behavior, but removes unnecessary hole
initialization in a few places, most notably for the name binding
in named function expressions and for block-scoped function
declarations.

R=bmeurer@chromium.org, neis@chromium.org
==========

to

==========
Use Variable::binding_needs_init() to determine hole initialization

The old code was using VariableMode, but that signal is both
over-pessimistic (some CONST and LET variables need no hole-initialization)
and inconsistent with other uses of the InitializationFlag enum (such
as %LoadLookupSlot).

This changes no observable behavior, but removes unnecessary hole
initialization and hole checks in a few places, most notably for
the name binding in named function expressions and for
block-scoped function declarations.

R=bmeurer@chromium.org, neis@chromium.org
==========

[adamk, 2016-08-03 22:38:01]

Description was changed from

==========
Use Variable::binding_needs_init() to determine hole initialization

The old code was using VariableMode, but that signal is both
over-pessimistic (some CONST and LET variables need no hole-initialization)
and inconsistent with other uses of the InitializationFlag enum (such
as %LoadLookupSlot).

This changes no observable behavior, but removes unnecessary hole
initialization and hole checks in a few places, most notably for
the name binding in named function expressions and for
block-scoped function declarations.

R=bmeurer@chromium.org, neis@chromium.org
==========

to

==========
Use Variable::binding_needs_init() to determine hole initialization

The old code was using VariableMode, but that signal is both
over-pessimistic (some CONST and LET variables need no hole-initialization)
and inconsistent with other uses of the InitializationFlag enum (such
as %LoadLookupSlot).

This changes no observable behavior, but removes unnecessary hole
initialization and hole checks in a few places, including
block-scoped function declarations and new.target.

R=bmeurer@chromium.org, neis@chromium.org
==========

[adamk, 2016-08-03 22:52:58]

Description was changed from

==========
Use Variable::binding_needs_init() to determine hole initialization

The old code was using VariableMode, but that signal is both
over-pessimistic (some CONST and LET variables need no hole-initialization)
and inconsistent with other uses of the InitializationFlag enum (such
as %LoadLookupSlot).

This changes no observable behavior, but removes unnecessary hole
initialization and hole checks in a few places, including
block-scoped function declarations and new.target.

R=bmeurer@chromium.org, neis@chromium.org
==========

to

==========
Use Variable::binding_needs_init() to determine hole initialization

The old code was using VariableMode, but that signal is both
over-pessimistic (some CONST and LET variables need no hole-initialization)
and inconsistent with other uses of the InitializationFlag enum (such
as %LoadLookupSlot).

This changes no observable behavior, but removes unnecessary hole
initialization and hole checks in a few places, including
block-scoped function declarations, .this_function, and new.target.

R=bmeurer@chromium.org, neis@chromium.org
==========

[adamk, 2016-08-03 22:53:31]

Description was changed from

==========
Use Variable::binding_needs_init() to determine hole initialization

The old code was using VariableMode, but that signal is both
over-pessimistic (some CONST and LET variables need no hole-initialization)
and inconsistent with other uses of the InitializationFlag enum (such
as %LoadLookupSlot).

This changes no observable behavior, but removes unnecessary hole
initialization and hole checks in a few places, including
block-scoped function declarations, .this_function, and new.target.

R=bmeurer@chromium.org, neis@chromium.org
==========

to

==========
Use Variable::binding_needs_init() to determine hole initialization

The old code was using VariableMode, but that signal is both
over-pessimistic (some CONST and LET variables need no hole-initialization)
and inconsistent with other uses of the InitializationFlag enum (such
as %LoadLookupSlot).

This changes no observable behavior, but removes unnecessary hole
initialization and hole checks in a few places, including
block-scoped function declarations, super property lookups,
and new.target.

R=bmeurer@chromium.org, neis@chromium.org
==========

[adamk, 2016-08-03 23:35:33]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 23:35:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adamk, 2016-08-03 23:36:46]

Okay, this patch now addresses all hole checking in the backend AST visitors.

I considered not making changes to full-codegen, but it seemed worse to be
inconsistent across backends than to make some modifications to full-codegen.

[adamk, 2016-08-03 23:38:55]

A few notes

https://codereview.chromium.org/2201193004/diff/50018/src/full-codegen/x64/fu...
File src/full-codegen/x64/full-codegen-x64.cc (left):

https://codereview.chromium.org/2201193004/diff/50018/src/full-codegen/x64/fu...
src/full-codegen/x64/full-codegen-x64.cc:1280: if (var->mode() == LET ||
var->mode() == CONST) {
There should be no way for us to get into this block if we're not either LET or
CONST.

https://codereview.chromium.org/2201193004/diff/50018/src/full-codegen/x64/fu...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/2201193004/diff/50018/src/full-codegen/x64/fu...
src/full-codegen/x64/full-codegen-x64.cc:1226: } else {
We previously always emitted the jmp below, even when it was unreachable due to
the Throw above.

[commit-bot: I haz the power, 2016-08-03 23:51:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-03 23:51:14]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6286)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[adamk, 2016-08-04 00:03:23]

The CQ bit was checked by adamk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 00:03:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 00:23:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 00:23:46]

Dry run: This issue passed the CQ dry run.

[neis, 2016-08-04 09:27:12]

Great! lgtm

https://codereview.chromium.org/2201193004/diff/70001/src/full-codegen/x64/fu...
File src/full-codegen/x64/full-codegen-x64.cc (left):

https://codereview.chromium.org/2201193004/diff/70001/src/full-codegen/x64/fu...
src/full-codegen/x64/full-codegen-x64.cc:726: VariableMode mode =
declaration->mode();
I suppose the declaration's mode is always in sync with the variable's mode? Can
we remove the mode_ field from Declaration then?

https://codereview.chromium.org/2201193004/diff/70001/src/full-codegen/x64/fu...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/2201193004/diff/70001/src/full-codegen/x64/fu...
src/full-codegen/x64/full-codegen-x64.cc:2076: // Assignment to const is
exception in all modes.
"modes" in this comment is confusing.

[rmcilroy, 2016-08-04 14:15:05]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[rmcilroy, 2016-08-04 14:15:06]

Interpreter lgtm, thanks.

[adamk, 2016-08-04 17:46:59]

bmeurer, ptal for compiler/ and crankshaft/

https://codereview.chromium.org/2201193004/diff/70001/src/full-codegen/x64/fu...
File src/full-codegen/x64/full-codegen-x64.cc (left):

https://codereview.chromium.org/2201193004/diff/70001/src/full-codegen/x64/fu...
src/full-codegen/x64/full-codegen-x64.cc:726: VariableMode mode =
declaration->mode();
On 2016/08/04 09:27:12, neis wrote:
> I suppose the declaration's mode is always in sync with the variable's mode?
Can
> we remove the mode_ field from Declaration then?

Yes, as discussed offline it seems like this can die (in a separate CL).

https://codereview.chromium.org/2201193004/diff/70001/src/full-codegen/x64/fu...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/2201193004/diff/70001/src/full-codegen/x64/fu...
src/full-codegen/x64/full-codegen-x64.cc:2076: // Assignment to const is
exception in all modes.
On 2016/08/04 09:27:12, neis wrote:
> "modes" in this comment is confusing.

Agreed, just removed it (pretty clear what it's doing, I think).

[Benedikt Meurer, 2016-08-05 05:09:41]

LGTM.

[neis, 2016-08-05 07:23:38]

On 2016/08/04 17:46:59, adamk wrote:
> bmeurer, ptal for compiler/ and crankshaft/
> 
>
https://codereview.chromium.org/2201193004/diff/70001/src/full-codegen/x64/fu...
> File src/full-codegen/x64/full-codegen-x64.cc (left):
> 
>
https://codereview.chromium.org/2201193004/diff/70001/src/full-codegen/x64/fu...
> src/full-codegen/x64/full-codegen-x64.cc:726: VariableMode mode =
> declaration->mode();
> On 2016/08/04 09:27:12, neis wrote:
> > I suppose the declaration's mode is always in sync with the variable's mode?
> Can
> > we remove the mode_ field from Declaration then?
> 
> Yes, as discussed offline it seems like this can die (in a separate CL).
> 
>
https://codereview.chromium.org/2201193004/diff/70001/src/full-codegen/x64/fu...
> File src/full-codegen/x64/full-codegen-x64.cc (right):
> 
>
https://codereview.chromium.org/2201193004/diff/70001/src/full-codegen/x64/fu...
> src/full-codegen/x64/full-codegen-x64.cc:2076: // Assignment to const is
> exception in all modes.
> On 2016/08/04 09:27:12, neis wrote:
> > "modes" in this comment is confusing.
> 
> Agreed, just removed it (pretty clear what it's doing, I think).

Thanks. The same comment exists in ast-graph-builder.cc.

[adamk, 2016-08-05 17:28:51]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-08-05 17:28:52]

The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org,
rmcilroy@chromium.org, bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2201193004/#ps110001
(title: "Remove one more comment")

[commit-bot: I haz the power, 2016-08-05 17:29:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 17:50:14]

Description was changed from

==========
Use Variable::binding_needs_init() to determine hole initialization

The old code was using VariableMode, but that signal is both
over-pessimistic (some CONST and LET variables need no hole-initialization)
and inconsistent with other uses of the InitializationFlag enum (such
as %LoadLookupSlot).

This changes no observable behavior, but removes unnecessary hole
initialization and hole checks in a few places, including
block-scoped function declarations, super property lookups,
and new.target.

R=bmeurer@chromium.org, neis@chromium.org
==========

to

==========
Use Variable::binding_needs_init() to determine hole initialization

The old code was using VariableMode, but that signal is both
over-pessimistic (some CONST and LET variables need no hole-initialization)
and inconsistent with other uses of the InitializationFlag enum (such
as %LoadLookupSlot).

This changes no observable behavior, but removes unnecessary hole
initialization and hole checks in a few places, including
block-scoped function declarations, super property lookups,
and new.target.

R=bmeurer@chromium.org, neis@chromium.org
==========

[commit-bot: I haz the power, 2016-08-05 17:50:15]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2016-08-05 17:51:24]

Description was changed from

==========
Use Variable::binding_needs_init() to determine hole initialization

The old code was using VariableMode, but that signal is both
over-pessimistic (some CONST and LET variables need no hole-initialization)
and inconsistent with other uses of the InitializationFlag enum (such
as %LoadLookupSlot).

This changes no observable behavior, but removes unnecessary hole
initialization and hole checks in a few places, including
block-scoped function declarations, super property lookups,
and new.target.

R=bmeurer@chromium.org, neis@chromium.org
==========

to

==========
Use Variable::binding_needs_init() to determine hole initialization

The old code was using VariableMode, but that signal is both
over-pessimistic (some CONST and LET variables need no hole-initialization)
and inconsistent with other uses of the InitializationFlag enum (such
as %LoadLookupSlot).

This changes no observable behavior, but removes unnecessary hole
initialization and hole checks in a few places, including
block-scoped function declarations, super property lookups,
and new.target.

R=bmeurer@chromium.org, neis@chromium.org

Committed: https://crrev.com/6768456db513936cb19c8cdc657a7cd6122584a9
Cr-Commit-Position: refs/heads/master@{#38395}
==========

[commit-bot: I haz the power, 2016-08-05 17:51:25]

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