[wasm] Several unrelated cleanups

[wasm] Several unrelated cleanups

Most are minor performance optimizations that aggregated while implementing
other changes. Those fixes will probably not be visible in perf graphs, but
they bothered me anyway.

R=titzer@chromium.org, ahaas@chromium.org

Review-Url: https://codereview.chromium.org/2714373003
Cr-Commit-Position: refs/heads/master@{#43535}
Committed: https://chromium.googlesource.com/v8/v8/+/fa8dba0e864c8c5baa41b51ec7681a3af98b1692

[Clemens Hammacher, 2017-02-27 12:07:58]

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

[commit-bot: I haz the power, 2017-02-27 12:08:05]

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, 2017-02-27 12:30:35]

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

[commit-bot: I haz the power, 2017-02-27 12:30:36]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)

[Clemens Hammacher, 2017-02-27 13:01:15]

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

[commit-bot: I haz the power, 2017-02-27 13:01:17]

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, 2017-02-27 13:28:45]

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

[commit-bot: I haz the power, 2017-02-27 13:28:46]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-02-27 13:33:18]

Description was changed from

==========
[wasm] Several unrelated cleanups

Most are minor performance optimizations that aggreated while implementing
other changes. Those fixes will probably not be visible in perf graphs, but
they bothered me anyway.

R=titzer@chromium.org, ahaas@chromium.org
==========

to

==========
[wasm] Several unrelated cleanups

Most are minor performance optimizations that aggregated while implementing
other changes. Those fixes will probably not be visible in perf graphs, but
they bothered me anyway.

R=titzer@chromium.org, ahaas@chromium.org
==========

[Clemens Hammacher, 2017-02-28 15:01:15]

Ping

[titzer, 2017-02-28 15:15:07]

https://codereview.chromium.org/2714373003/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2714373003/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:372: for (size_t i = 0, e =
sig->parameter_count(); i < e && !has_simd_; ++i) {
You could also just iterate from sig->parameter_count() and iterate until
wraparound.

https://codereview.chromium.org/2714373003/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:373: if (sig->GetParam(i) == wasm::kWasmS128)
has_simd_ = true;
And could also just break if found, instead of checking every iteration.

https://codereview.chromium.org/2714373003/diff/20001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2714373003/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:910: void UpdateDispatchTablesInternal(Isolate* isolate,
FixedArray* dispatch_tables,
Please don't unhandlify this.

[ahaas, 2017-02-28 18:11:40]

https://codereview.chromium.org/2714373003/diff/20001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2714373003/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1025: Handle<Code>
orig_code(Code::cast(code_table->get(i)), isolate_);
With this change you get rid of the CHECK in GetValueChecked. Is it guaranteed
that code_table will never return undefined?

[Clemens Hammacher, 2017-03-01 16:02:33]

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

[commit-bot: I haz the power, 2017-03-01 16:02:42]

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, 2017-03-01 16:20:24]

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

[commit-bot: I haz the power, 2017-03-01 16:20:25]

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

[Clemens Hammacher, 2017-03-01 16:21:39]

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

[commit-bot: I haz the power, 2017-03-01 16:21:44]

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

[Clemens Hammacher, 2017-03-01 16:41:12]

https://codereview.chromium.org/2714373003/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2714373003/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:372: for (size_t i = 0, e =
sig->parameter_count(); i < e && !has_simd_; ++i) {
On 2017/02/28 at 15:15:07, titzer wrote:
> You could also just iterate from sig->parameter_count() and iterate until
wraparound.

Done (iterate while > 0 and use "i-1" in the body, since i is unsigned).

https://codereview.chromium.org/2714373003/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:373: if (sig->GetParam(i) == wasm::kWasmS128)
has_simd_ = true;
On 2017/02/28 at 15:15:07, titzer wrote:
> And could also just break if found, instead of checking every iteration.

I expect this to be eliminated by the compiler (SSA & jump threading). Adding an
explicit break would add three lines two times :(

https://codereview.chromium.org/2714373003/diff/20001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2714373003/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:910: void UpdateDispatchTablesInternal(Isolate* isolate,
FixedArray* dispatch_tables,
On 2017/02/28 at 15:15:07, titzer wrote:
> Please don't unhandlify this.

Un-done ;)

https://codereview.chromium.org/2714373003/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1025: Handle<Code>
orig_code(Code::cast(code_table->get(i)), isolate_);
On 2017/02/28 at 18:11:39, ahaas wrote:
> With this change you get rid of the CHECK in GetValueChecked. Is it guaranteed
that code_table will never return undefined?

Yes, this is guaranteed here. And Code::cast still does a SLOW_DCHECK for
IsCode().

[commit-bot: I haz the power, 2017-03-01 16:47:14]

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

[commit-bot: I haz the power, 2017-03-01 16:47:15]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-03-01 16:52:58]

lgtm

[Clemens Hammacher, 2017-03-02 10:40:31]

The CQ bit was checked by clemensh@chromium.org

[commit-bot: I haz the power, 2017-03-02 10:40:40]

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, 2017-03-02 10:42:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488451231766810,
"parent_rev": "9fd418b96a67f3c83e36dcc0fa274ab71d21bf56", "commit_rev":
"fa8dba0e864c8c5baa41b51ec7681a3af98b1692"}

[commit-bot: I haz the power, 2017-03-02 10:42:27]

Description was changed from

==========
[wasm] Several unrelated cleanups

Most are minor performance optimizations that aggregated while implementing
other changes. Those fixes will probably not be visible in perf graphs, but
they bothered me anyway.

R=titzer@chromium.org, ahaas@chromium.org
==========

to

==========
[wasm] Several unrelated cleanups

Most are minor performance optimizations that aggregated while implementing
other changes. Those fixes will probably not be visible in perf graphs, but
they bothered me anyway.

R=titzer@chromium.org, ahaas@chromium.org

Review-Url: https://codereview.chromium.org/2714373003
Cr-Commit-Position: refs/heads/master@{#43535}
Committed:
https://chromium.googlesource.com/v8/v8/+/fa8dba0e864c8c5baa41b51ec7681a3af98...
==========

[commit-bot: I haz the power, 2017-03-02 10:42:28]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/fa8dba0e864c8c5baa41b51ec7681a3af98...