[ignition] Eliminate hole checks where statically possible for loads and stores

[ignition] Eliminate hole checks where statically possible for loads and stores

Move hole check logic from full-codegen into scope analysis, and store the
"needs hole check" bit on VariableProxy. This makes it easy to re-use in
any backend: it will be trivial to extend the use of this logic in, e.g.,
full-codegen variable stores.

While changing the signatures of the variable loading/storing methods in
Ignition, I took the liberty of replacing the verb "Visit" with "Build", since these
are not part of AST visiting.

BUG=v8:5460
Committed: https://crrev.com/35a3ccbfac415f7ab53ed5a63b03ed366d29a104
Cr-Commit-Position: refs/heads/master@{#40479}

[adamk, 2016-10-13 02:13:27]

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

[commit-bot: I haz the power, 2016-10-13 02:13:34]

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-10-13 02:28:03]

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

[commit-bot: I haz the power, 2016-10-13 02:28:04]

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

[adamk, 2016-10-14 22:47:02]

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

[commit-bot: I haz the power, 2016-10-14 22:47:11]

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

[adamk, 2016-10-14 22:52:43]

Description was changed from

==========
[ignition] Eliminate hole checks where statically possible for loads and stores

BUG=v8:5460
==========

to

==========
[ignition] Eliminate hole checks where statically possible for loads and stores

Move hole check logic from full-codegen into scope analysis, and store the
"needs hole check" bit on VariableProxy. This makes it easy to re-use in
any backend: it will be trivial to extend the use of this logic for, e.g.,
full-codegen
variable stores.

BUG=v8:5460
==========

[adamk, 2016-10-14 22:53:15]

adamk@chromium.org changed reviewers:
+ bmeurer@chromium.org, mstarzinger@chromium.org, mythria@chromium.org

[adamk, 2016-10-14 22:53:30]

Description was changed from

==========
[ignition] Eliminate hole checks where statically possible for loads and stores

Move hole check logic from full-codegen into scope analysis, and store the
"needs hole check" bit on VariableProxy. This makes it easy to re-use in
any backend: it will be trivial to extend the use of this logic for, e.g.,
full-codegen
variable stores.

BUG=v8:5460
==========

to

==========
[ignition] Eliminate hole checks where statically possible for loads and stores

Move hole check logic from full-codegen into scope analysis, and store the
"needs hole check" bit on VariableProxy. This makes it easy to re-use in
any backend: it will be trivial to extend the use of this logic for, e.g.,
full-codegen variable stores.

BUG=v8:5460
==========

[adamk, 2016-10-14 22:54:14]

Description was changed from

==========
[ignition] Eliminate hole checks where statically possible for loads and stores

Move hole check logic from full-codegen into scope analysis, and store the
"needs hole check" bit on VariableProxy. This makes it easy to re-use in
any backend: it will be trivial to extend the use of this logic for, e.g.,
full-codegen variable stores.

BUG=v8:5460
==========

to

==========
[ignition] Eliminate hole checks where statically possible for loads and stores

Move hole check logic from full-codegen into scope analysis, and store the
"needs hole check" bit on VariableProxy. This makes it easy to re-use in
any backend: it will be trivial to extend the use of this logic in, e.g.,
full-codegen variable stores.

BUG=v8:5460
==========

[adamk, 2016-10-14 22:55:35]

Description was changed from

==========
[ignition] Eliminate hole checks where statically possible for loads and stores

Move hole check logic from full-codegen into scope analysis, and store the
"needs hole check" bit on VariableProxy. This makes it easy to re-use in
any backend: it will be trivial to extend the use of this logic in, e.g.,
full-codegen variable stores.

BUG=v8:5460
==========

to

==========
[ignition] Eliminate hole checks where statically possible for loads and stores

Move hole check logic from full-codegen into scope analysis, and store the
"needs hole check" bit on VariableProxy. This makes it easy to re-use in
any backend: it will be trivial to extend the use of this logic in, e.g.,
full-codegen variable stores.

While changing the signatures of the variable loading/storing methods in
Ignition, I took the liberty of replacing the verb "Visit" with "Build", since
these
are not part of AST visiting.

BUG=v8:5460
==========

[commit-bot: I haz the power, 2016-10-14 23:07:10]

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

[commit-bot: I haz the power, 2016-10-14 23:07:11]

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/10869)
  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-10-15 01:05:27]

It appears this approach almost works, but that I'm somehow capturing too any
variables in this check. I'd still be interested in comments on the overall
approach, as I suspect it will be possible to filter out the places where
DCHECKs are failing.

[Michael Starzinger, 2016-10-17 14:15:35]

I like the approach, back-ends look good, fine from my end. Adding Toon to
double check that setting the bit in Scope::ResolveTo is sound.

https://codereview.chromium.org/2411873004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/2411873004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:97: void BuildVariableLoad(VariableProxy*
proxy,
nit: Not a huge fan of having to pass in the variable proxy for loading. I know
boolean flags are also ugly, but in this case I would actually prefer
BuildVariableLoad(Variable*, FeedbackVectorSlot, bool) as a signature. Similar
for the other methods below. Especially in light of BuildVariableAssignment
being used in places where no valid proxy is available. WDYT?

[Michael Starzinger, 2016-10-17 14:15:54]

mstarzinger@chromium.org changed reviewers:
+ verwaest@chromium.org

[Michael Starzinger, 2016-10-17 14:15:54]

Actually adding Toon. :)

[mythria, 2016-10-18 06:43:37]

Nice, looks good from my side.

[adamk, 2016-10-19 10:38:51]

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

[commit-bot: I haz the power, 2016-10-19 10:39:03]

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-10-19 10:41:32]

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

[commit-bot: I haz the power, 2016-10-19 10:41:33]

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/11107)

[adamk, 2016-10-19 10:55:33]

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

[commit-bot: I haz the power, 2016-10-19 10:55:40]

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

[adamk, 2016-10-19 11:09:43]

https://codereview.chromium.org/2411873004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/2411873004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:97: void BuildVariableLoad(VariableProxy*
proxy,
On 2016/10/17 14:15:35, Michael Starzinger wrote:
> nit: Not a huge fan of having to pass in the variable proxy for loading. I
know
> boolean flags are also ugly, but in this case I would actually prefer
> BuildVariableLoad(Variable*, FeedbackVectorSlot, bool) as a signature. Similar
> for the other methods below. Especially in light of BuildVariableAssignment
> being used in places where no valid proxy is available. WDYT?

Do you mean BuildVariableAssignment? I agree there's ugliness in having to pass
both a variable and a proxy there, and a bool isn't really any worse (and I'm
happy to replace that fourth argument). But for BuildVariableLoad passing a
proxy seems strictly simpler. Why are you not a fan of passing a proxy there?

[commit-bot: I haz the power, 2016-10-19 11:17:30]

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

[commit-bot: I haz the power, 2016-10-19 11:17:31]

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

[adamk, 2016-10-19 11:31:32]

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

[commit-bot: I haz the power, 2016-10-19 11:31: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-10-19 11:37:14]

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

[commit-bot: I haz the power, 2016-10-19 11:37:24]

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-10-19 12:00:55]

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

[commit-bot: I haz the power, 2016-10-19 12:00:56]

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

[adamk, 2016-10-19 12:24:08]

This is now working except for module imports: the existing code was depending
on having variable allocation information available before testing for
NeedsHoleCheck, but allocation happens after resolution. 

This is fixable, but I'm not sure yet the cleanest way: the brute force solution
would be to allocate imports when they're declared, but that might be kind of
ugly.

[Michael Starzinger, 2016-10-19 12:32:41]

https://codereview.chromium.org/2411873004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/2411873004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:97: void BuildVariableLoad(VariableProxy*
proxy,
On 2016/10/19 11:09:43, adamk wrote:
> On 2016/10/17 14:15:35, Michael Starzinger wrote:
> > nit: Not a huge fan of having to pass in the variable proxy for loading. I
> know
> > boolean flags are also ugly, but in this case I would actually prefer
> > BuildVariableLoad(Variable*, FeedbackVectorSlot, bool) as a signature.
Similar
> > for the other methods below. Especially in light of BuildVariableAssignment
> > being used in places where no valid proxy is available. WDYT?
> 
> Do you mean BuildVariableAssignment? I agree there's ugliness in having to
pass
> both a variable and a proxy there, and a bool isn't really any worse (and I'm
> happy to replace that fourth argument). But for BuildVariableLoad passing a
> proxy seems strictly simpler. Why are you not a fan of passing a proxy there?

Yes, my comment was inspired by the "ugliness" in BuildVariableAssignment. And
then I extended my train of thought to all three methods. My goal was to keep
them in sync so that they all either operate on the {Variable} level or on the
{VariableProxy} level, instead of a mixed API where loading works on
{VariableProxy} and assignment works on {Variable}.

Not too strong of an opinion and I can also live with how it is now. So feel
free to ignore my nit here.

[adamk, 2016-10-19 13:04:12]

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

[commit-bot: I haz the power, 2016-10-19 13:04:18]

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

[adamk, 2016-10-19 13:33:55]

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

[commit-bot: I haz the power, 2016-10-19 13:33:57]

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

[adamk, 2016-10-19 13:34:20]

adamk@chromium.org changed reviewers:
+ neis@chromium.org

[adamk, 2016-10-19 13:34:23]

I've got the modules tests passing, adding neis@ for modules. PTAL.

https://codereview.chromium.org/2411873004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/2411873004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:97: void BuildVariableLoad(VariableProxy*
proxy,
On 2016/10/19 12:32:41, Michael Starzinger wrote:
> On 2016/10/19 11:09:43, adamk wrote:
> > On 2016/10/17 14:15:35, Michael Starzinger wrote:
> > > nit: Not a huge fan of having to pass in the variable proxy for loading. I
> > know
> > > boolean flags are also ugly, but in this case I would actually prefer
> > > BuildVariableLoad(Variable*, FeedbackVectorSlot, bool) as a signature.
> Similar
> > > for the other methods below. Especially in light of
BuildVariableAssignment
> > > being used in places where no valid proxy is available. WDYT?
> > 
> > Do you mean BuildVariableAssignment? I agree there's ugliness in having to
> pass
> > both a variable and a proxy there, and a bool isn't really any worse (and
I'm
> > happy to replace that fourth argument). But for BuildVariableLoad passing a
> > proxy seems strictly simpler. Why are you not a fan of passing a proxy
there?
> 
> Yes, my comment was inspired by the "ugliness" in BuildVariableAssignment. And
> then I extended my train of thought to all three methods. My goal was to keep
> them in sync so that they all either operate on the {Variable} level or on the
> {VariableProxy} level, instead of a mixed API where loading works on
> {VariableProxy} and assignment works on {Variable}.
> 
> Not too strong of an opinion and I can also live with how it is now. So feel
> free to ignore my nit here.

I can see the argument for consistency, but it does make the callers of
BuildVariableLoad rather verbose. See what you think of patchset 8.

[commit-bot: I haz the power, 2016-10-19 14:01:46]

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

[commit-bot: I haz the power, 2016-10-19 14:01:47]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-10-19 14:05:33]

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

[commit-bot: I haz the power, 2016-10-19 14:05:37]

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-10-19 14:35:34]

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

[commit-bot: I haz the power, 2016-10-19 14:35:35]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-10-19 15:41:52]

LGTM from my end.

https://codereview.chromium.org/2411873004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/2411873004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:97: void BuildVariableLoad(VariableProxy*
proxy,
On 2016/10/19 13:34:23, adamk wrote:
> On 2016/10/19 12:32:41, Michael Starzinger wrote:
> > On 2016/10/19 11:09:43, adamk wrote:
> > > On 2016/10/17 14:15:35, Michael Starzinger wrote:
> > > > nit: Not a huge fan of having to pass in the variable proxy for loading.
I
> > > know
> > > > boolean flags are also ugly, but in this case I would actually prefer
> > > > BuildVariableLoad(Variable*, FeedbackVectorSlot, bool) as a signature.
> > Similar
> > > > for the other methods below. Especially in light of
> BuildVariableAssignment
> > > > being used in places where no valid proxy is available. WDYT?
> > > 
> > > Do you mean BuildVariableAssignment? I agree there's ugliness in having to
> > pass
> > > both a variable and a proxy there, and a bool isn't really any worse (and
> I'm
> > > happy to replace that fourth argument). But for BuildVariableLoad passing
a
> > > proxy seems strictly simpler. Why are you not a fan of passing a proxy
> there?
> > 
> > Yes, my comment was inspired by the "ugliness" in BuildVariableAssignment.
And
> > then I extended my train of thought to all three methods. My goal was to
keep
> > them in sync so that they all either operate on the {Variable} level or on
the
> > {VariableProxy} level, instead of a mixed API where loading works on
> > {VariableProxy} and assignment works on {Variable}.
> > 
> > Not too strong of an opinion and I can also live with how it is now. So feel
> > free to ignore my nit here.
> 
> I can see the argument for consistency, but it does make the callers of
> BuildVariableLoad rather verbose. See what you think of patchset 8.

I would be fine with patch set #8.

[Toon Verwaest, 2016-10-20 12:41:22]

I presume the original reason why I was added here is already gone by now, but
lgtm nonetheless (also not a terrible fan of the bools I must say :))

[neis, 2016-10-20 16:46:42]

lgtm with comment

https://codereview.chromium.org/2411873004/diff/160001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2411873004/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1924: var->AllocateTo(VariableLocation::MODULE, 0);
You can use your new enum constant here.

[adamk, 2016-10-20 16:53:25]

Latest patch set addresses neis's concerns, it's likely the allocation logic
will change in a followup.

https://codereview.chromium.org/2411873004/diff/160001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2411873004/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1924: var->AllocateTo(VariableLocation::MODULE, 0);
On 2016/10/20 16:46:42, neis wrote:
> You can use your new enum constant here.

Done.

[adamk, 2016-10-20 16:55:55]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-10-20 16:55:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, neis@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2411873004/#ps200001
(title: "Rebased")

[commit-bot: I haz the power, 2016-10-20 16:56:00]

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-10-20 17:31:43]

Description was changed from

==========
[ignition] Eliminate hole checks where statically possible for loads and stores

Move hole check logic from full-codegen into scope analysis, and store the
"needs hole check" bit on VariableProxy. This makes it easy to re-use in
any backend: it will be trivial to extend the use of this logic in, e.g.,
full-codegen variable stores.

While changing the signatures of the variable loading/storing methods in
Ignition, I took the liberty of replacing the verb "Visit" with "Build", since
these
are not part of AST visiting.

BUG=v8:5460
==========

to

==========
[ignition] Eliminate hole checks where statically possible for loads and stores

Move hole check logic from full-codegen into scope analysis, and store the
"needs hole check" bit on VariableProxy. This makes it easy to re-use in
any backend: it will be trivial to extend the use of this logic in, e.g.,
full-codegen variable stores.

While changing the signatures of the variable loading/storing methods in
Ignition, I took the liberty of replacing the verb "Visit" with "Build", since
these
are not part of AST visiting.

BUG=v8:5460
==========

[commit-bot: I haz the power, 2016-10-20 17:31:44]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-11-17 22:08:37]

Description was changed from

==========
[ignition] Eliminate hole checks where statically possible for loads and stores

Move hole check logic from full-codegen into scope analysis, and store the
"needs hole check" bit on VariableProxy. This makes it easy to re-use in
any backend: it will be trivial to extend the use of this logic in, e.g.,
full-codegen variable stores.

While changing the signatures of the variable loading/storing methods in
Ignition, I took the liberty of replacing the verb "Visit" with "Build", since
these
are not part of AST visiting.

BUG=v8:5460
==========

to

==========
[ignition] Eliminate hole checks where statically possible for loads and stores

Move hole check logic from full-codegen into scope analysis, and store the
"needs hole check" bit on VariableProxy. This makes it easy to re-use in
any backend: it will be trivial to extend the use of this logic in, e.g.,
full-codegen variable stores.

While changing the signatures of the variable loading/storing methods in
Ignition, I took the liberty of replacing the verb "Visit" with "Build", since
these
are not part of AST visiting.

BUG=v8:5460

Committed: https://crrev.com/35a3ccbfac415f7ab53ed5a63b03ed366d29a104
Cr-Commit-Position: refs/heads/master@{#40479}
==========

[commit-bot: I haz the power, 2016-11-17 22:08:39]

Patchset 11 (id:??) landed as
https://crrev.com/35a3ccbfac415f7ab53ed5a63b03ed366d29a104
Cr-Commit-Position: refs/heads/master@{#40479}