[interpreter][stubs] Fixing issues found by machine graph verifier.

[interpreter][stubs] Fixing issues found by machine graph verifier.

All issues in interpreter bytecode handlers are fixed.

BUG=

Committed: https://crrev.com/02f917f7ef4d2f7c63346002d5eae5375c3474c1
Cr-Commit-Position: refs/heads/master@{#41649}

[Igor Sheludko, 2016-12-08 22:41:08]

Description was changed from

==========
fixing further

fixing interpreter

moving forward with the interpreter

fox

Moving forward

Moving forward

Moving forward

still moving

bytecode handlers seems to work

LoadConstantPool test fixed

formatted

BUG=
==========

to

==========
[interpreter][stubs] Fixing issues found by machine graph verifier.

BUG=
==========

[Igor Sheludko, 2016-12-09 09:58:51]

Patchset #1 (id:1) has been deleted

[Igor Sheludko, 2016-12-09 10:18:58]

Patchset #1 (id:20001) has been deleted

[Igor Sheludko, 2016-12-09 10:19:06]

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

[commit-bot: I haz the power, 2016-12-09 10:19:15]

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

[Igor Sheludko, 2016-12-09 10:20:39]

The CQ bit was unchecked by ishell@chromium.org

[Igor Sheludko, 2016-12-09 10:22:14]

ishell@chromium.org changed reviewers:
+ cbruni@chromium.org, epertoso@chromium.org, rmcilroy@chromium.org

[Igor Sheludko, 2016-12-09 10:22:15]

Enrico, PTAL @ verifier
Ross, PTAL @ interpreter and tests
Camillo, PTAL @ the rest

[rmcilroy, 2016-12-09 11:21:37]

LGTM, thanks.

https://codereview.chromium.org/2552883012/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2552883012/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:215: return ChangeUint32ToWord(load);
Not something for this CL (unless it's easier) but would it make sense to treat
nodes loaded from operands as Word32 instead of Word? The max size of an operand
is 32bit, and I'm wondering if it would avoid all the conversion here and
possibly cause less conversion in operators which use the results as if they are
Int32. WDYT?

https://codereview.chromium.org/2552883012/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:1036: IntPtrConstant(Code::kHeaderSize
- kHeapObjectTag));
nit - Maybe there should be a CSA helper which converts an Object* to a Address
at a particular offset in the object?

https://codereview.chromium.org/2552883012/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:1297: INTPTR_PARAMETERS);
Do you need the extra arguments here, aren't these the default (it's actually
super confusing that the default for PARAMETERS_ TYPES gave different defaults
for the two overloads - could we fix that?).

[Camillo Bruni, 2016-12-09 11:52:14]

LGTM, nice nice :)

[epertoso, 2016-12-09 13:10:02]

lgtm for compiler/, thanks.

[Igor Sheludko, 2016-12-09 17:46:19]

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

[commit-bot: I haz the power, 2016-12-09 17:46:20]

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-12-09 18:05:18]

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

[commit-bot: I haz the power, 2016-12-09 18:05:19]

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...)

[Igor Sheludko, 2016-12-09 20:29:11]

Patchset #2 (id:60001) has been deleted

[Igor Sheludko, 2016-12-09 20:29:35]

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

[commit-bot: I haz the power, 2016-12-09 20:29:41]

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-12-09 21:22:15]

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

[commit-bot: I haz the power, 2016-12-09 21:22:16]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2016-12-09 23:58:02]

Patchset #3 (id:100001) has been deleted

[Igor Sheludko, 2016-12-09 23:58:10]

Patchset #2 (id:80001) has been deleted

[Igor Sheludko, 2016-12-10 00:05:46]

Patchset #2 (id:120001) has been deleted

[Igor Sheludko, 2016-12-10 00:07:18]

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

[commit-bot: I haz the power, 2016-12-10 00:07:28]

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

[Igor Sheludko, 2016-12-10 00:08:42]

Description was changed from

==========
[interpreter][stubs] Fixing issues found by machine graph verifier.

BUG=
==========

to

==========
[interpreter][stubs] Fixing issues found by machine graph verifier.

All issues in interpreter bytecode handlers are fixed.

BUG=
==========

[Igor Sheludko, 2016-12-10 00:09:50]

Ross, PTAL again. I addressed your comments.

https://codereview.chromium.org/2552883012/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2552883012/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:215: return ChangeUint32ToWord(load);
On 2016/12/09 11:21:37, rmcilroy wrote:
> Not something for this CL (unless it's easier) but would it make sense to
treat
> nodes loaded from operands as Word32 instead of Word? The max size of an
operand
> is 32bit, and I'm wondering if it would avoid all the conversion here and
> possibly cause less conversion in operators which use the results as if they
are
> Int32. WDYT?

Done.

https://codereview.chromium.org/2552883012/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:1036: IntPtrConstant(Code::kHeaderSize
- kHeapObjectTag));
On 2016/12/09 11:21:37, rmcilroy wrote:
> nit - Maybe there should be a CSA helper which converts an Object* to a
Address
> at a particular offset in the object?

Added TODO, will address in a follow-up CL.

https://codereview.chromium.org/2552883012/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:1297: INTPTR_PARAMETERS);
On 2016/12/09 11:21:37, rmcilroy wrote:
> Do you need the extra arguments here, aren't these the default (it's actually
> super confusing that the default for PARAMETERS_ TYPES gave different defaults
> for the two overloads - could we fix that?).

Currently, all Load/StoreFixedXXXArray methods expect INTEGER_PARAMETERS (which
is int32, requiring sign extension) by default. The plan is to remove
INTEGER_PARAMETERS completely once we fix all the callsites, make
INTPTR_PARAMETERS default and then switch all the callsites that pass intptr
values back to using defaults.

[commit-bot: I haz the power, 2016-12-10 00:47:43]

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

[commit-bot: I haz the power, 2016-12-10 00:47:44]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-12-12 14:09:08]

LGTM, thanks!

https://codereview.chromium.org/2552883012/diff/140001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2552883012/diff/140001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:857: void
InterpreterAssembler::UpdateInterruptBudget(Node* weight32) {
nit - drop the 32

https://codereview.chromium.org/2552883012/diff/140001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/2552883012/diff/140001/src/interpreter/interp...
src/interpreter/interpreter-assembler.h:232: void
UpdateInterruptBudget(compiler::Node* weight32);
nit - just call this weight and update the comment with the type expected.

[Igor Sheludko, 2016-12-12 14:18:32]

Thanks, landing!

https://codereview.chromium.org/2552883012/diff/140001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2552883012/diff/140001/src/interpreter/interp...
src/interpreter/interpreter-assembler.cc:857: void
InterpreterAssembler::UpdateInterruptBudget(Node* weight32) {
On 2016/12/12 14:09:08, rmcilroy wrote:
> nit - drop the 32

Done.

https://codereview.chromium.org/2552883012/diff/140001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/2552883012/diff/140001/src/interpreter/interp...
src/interpreter/interpreter-assembler.h:232: void
UpdateInterruptBudget(compiler::Node* weight32);
On 2016/12/12 14:09:08, rmcilroy wrote:
> nit - just call this weight and update the comment with the type expected.

Done.

[Igor Sheludko, 2016-12-12 14:23:56]

The CQ bit was checked by ishell@chromium.org

[Igor Sheludko, 2016-12-12 14:23:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
epertoso@chromium.org, cbruni@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2552883012/#ps160001
(title: "Addressing nits")

[commit-bot: I haz the power, 2016-12-12 14:24:13]

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-12-12 14:52:33]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481552636576740,
"parent_rev": "27afcf3f76ad41e7ce38f9a91b0a747907511892", "commit_rev":
"869bd90be7acfbd11268729af830e8876c74efdd"}

[commit-bot: I haz the power, 2016-12-12 14:52:36]

Description was changed from

==========
[interpreter][stubs] Fixing issues found by machine graph verifier.

All issues in interpreter bytecode handlers are fixed.

BUG=
==========

to

==========
[interpreter][stubs] Fixing issues found by machine graph verifier.

All issues in interpreter bytecode handlers are fixed.

BUG=

Review-Url: https://codereview.chromium.org/2552883012
==========

[commit-bot: I haz the power, 2016-12-12 14:52:37]

Committed patchset #3 (id:160001)

[commit-bot: I haz the power, 2016-12-12 14:53:11]

Description was changed from

==========
[interpreter][stubs] Fixing issues found by machine graph verifier.

All issues in interpreter bytecode handlers are fixed.

BUG=

Review-Url: https://codereview.chromium.org/2552883012
==========

to

==========
[interpreter][stubs] Fixing issues found by machine graph verifier.

All issues in interpreter bytecode handlers are fixed.

BUG=

Committed: https://crrev.com/02f917f7ef4d2f7c63346002d5eae5375c3474c1
Cr-Commit-Position: refs/heads/master@{#41649}
==========

[commit-bot: I haz the power, 2016-12-12 14:53:12]

Patchset 3 (id:??) landed as
https://crrev.com/02f917f7ef4d2f7c63346002d5eae5375c3474c1
Cr-Commit-Position: refs/heads/master@{#41649}