[turbofan] Introduce integer multiplication with overflow.

[turbofan] Introduce integer multiplication with overflow.

BUG=

Committed: https://crrev.com/8e18a5f2a0357c6899cd32f5ab66b73d72fdd996
Cr-Commit-Position: refs/heads/master@{#37748}

[mvstanton, 2016-07-11 14:53:17]

mvstanton@chromium.org changed reviewers:
+ bmeurer@chromium.org

[mvstanton, 2016-07-11 14:53:18]

Hi Benedikt, here is the CL we discussed, thanks for the look.

Mips folks, could you guys make the mips/mips64 port for this case?

Thanks all for the help,
--Mike

[Benedikt Meurer, 2016-07-12 04:09:28]

Looking good, thanks man. First round of comments.

https://codereview.chromium.org/2101123005/diff/100001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2101123005/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:1070: Label end(assembler);
Nit: Can you rename this to return_result and add it to the preceeding line,
i.e.:

Label do_fmul(assembler), return_result(assembler);

https://codereview.chromium.org/2101123005/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:1102: // TODO(epertoso): use SmiMulWithOverflow once
available.
Remove this TODO.

https://codereview.chromium.org/2101123005/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:1110: Label if_overflow(assembler), if_notoverflow(assembler);
Make the if_overflow label deferred.

https://codereview.chromium.org/2101123005/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:1113: assembler->Bind(&if_notoverflow);
Add braces around the actual blocks for improved readability.

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/arm/instr...
File src/compiler/arm/instruction-selector-arm.cc (right):

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/arm/instr...
src/compiler/arm/instruction-selector-arm.cc:1096: void
VisitInt32MulWithOverflowImpl(InstructionSelector* selector, Node* node,
Put this into an anonymous namespace and remove the Impl suffix.

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/arm/instr...
src/compiler/arm/instruction-selector-arm.cc:1712: // ARM doesn't set the
overflow flag for multiplication, so we
Nice, thanks.

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/arm64/ins...
File src/compiler/arm64/instruction-selector-arm64.cc (right):

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/arm64/ins...
src/compiler/arm64/instruction-selector-arm64.cc:1273: void
VisitInt32MulWithOverflowImpl(InstructionSelector* selector, Node* node,
Put this into an anonymous namespace and remove the Impl suffix.

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/machine-o...
File src/compiler/machine-operator-reducer.cc (right):

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/machine-o...
src/compiler/machine-operator-reducer.cc:818: // TODO(mvstanton): put
kInt32MulWithOverflow here too, eh?
Nope, that doesn't make sense. The rules for the shift/and above are unnecessary
extensions/truncations.

https://codereview.chromium.org/2101123005/diff/100001/test/mjsunit/math-mul.js
File test/mjsunit/math-mul.js (right):

https://codereview.chromium.org/2101123005/diff/100001/test/mjsunit/math-mul....
test/mjsunit/math-mul.js:1: // Copyright 2013 the V8 project authors. All rights
reserved.
Nit: Old copyright header.

https://codereview.chromium.org/2101123005/diff/100001/test/mjsunit/math-mul....
test/mjsunit/math-mul.js:28: // Flags: --ignition
Don't pass Flags: here, Ignition is one of our test variants anyways.

[mvstanton, 2016-07-12 08:52:49]

Hi Benedikt, comments addressed, thanks!
--Mike

https://codereview.chromium.org/2101123005/diff/100001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2101123005/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:1070: Label end(assembler);
On 2016/07/12 04:09:28, Benedikt Meurer wrote:
> Nit: Can you rename this to return_result and add it to the preceeding line,
> i.e.:
> 
> Label do_fmul(assembler), return_result(assembler);

Done.

https://codereview.chromium.org/2101123005/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:1102: // TODO(epertoso): use SmiMulWithOverflow once
available.
On 2016/07/12 04:09:28, Benedikt Meurer wrote:
> Remove this TODO.

Done.

https://codereview.chromium.org/2101123005/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:1110: Label if_overflow(assembler), if_notoverflow(assembler);
On 2016/07/12 04:09:28, Benedikt Meurer wrote:
> Make the if_overflow label deferred.

Done.

https://codereview.chromium.org/2101123005/diff/100001/src/code-stubs.cc#newc...
src/code-stubs.cc:1113: assembler->Bind(&if_notoverflow);
On 2016/07/12 04:09:28, Benedikt Meurer wrote:
> Add braces around the actual blocks for improved readability.

Done.

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/arm/instr...
File src/compiler/arm/instruction-selector-arm.cc (right):

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/arm/instr...
src/compiler/arm/instruction-selector-arm.cc:1096: void
VisitInt32MulWithOverflowImpl(InstructionSelector* selector, Node* node,
On 2016/07/12 04:09:28, Benedikt Meurer wrote:
> Put this into an anonymous namespace and remove the Impl suffix.

Done.

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/arm/instr...
src/compiler/arm/instruction-selector-arm.cc:1712: // ARM doesn't set the
overflow flag for multiplication, so we
On 2016/07/12 04:09:28, Benedikt Meurer wrote:
> Nice, thanks.

Acknowledged.

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/arm64/ins...
File src/compiler/arm64/instruction-selector-arm64.cc (right):

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/arm64/ins...
src/compiler/arm64/instruction-selector-arm64.cc:1273: void
VisitInt32MulWithOverflowImpl(InstructionSelector* selector, Node* node,
On 2016/07/12 04:09:28, Benedikt Meurer wrote:
> Put this into an anonymous namespace and remove the Impl suffix.

Done.

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/machine-o...
File src/compiler/machine-operator-reducer.cc (right):

https://codereview.chromium.org/2101123005/diff/100001/src/compiler/machine-o...
src/compiler/machine-operator-reducer.cc:818: // TODO(mvstanton): put
kInt32MulWithOverflow here too, eh?
On 2016/07/12 04:09:28, Benedikt Meurer wrote:
> Nope, that doesn't make sense. The rules for the shift/and above are
unnecessary
> extensions/truncations.

Done.

https://codereview.chromium.org/2101123005/diff/100001/test/mjsunit/math-mul.js
File test/mjsunit/math-mul.js (right):

https://codereview.chromium.org/2101123005/diff/100001/test/mjsunit/math-mul....
test/mjsunit/math-mul.js:1: // Copyright 2013 the V8 project authors. All rights
reserved.
On 2016/07/12 04:09:28, Benedikt Meurer wrote:
> Nit: Old copyright header.

Done.

https://codereview.chromium.org/2101123005/diff/100001/test/mjsunit/math-mul....
test/mjsunit/math-mul.js:28: // Flags: --ignition
On 2016/07/12 04:09:28, Benedikt Meurer wrote:
> Don't pass Flags: here, Ignition is one of our test variants anyways.

Done.

[Benedikt Meurer, 2016-07-12 18:23:43]

Thanks, LGTM.

[Marija Antic, 2016-07-13 16:26:23]

MIPS port available https://codereview.chromium.org/2141063004

[mvstanton, 2016-07-13 21:16:17]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2016-07-13 21:16:18]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2101123005/#ps160001
(title: "MIPS port.")

[commit-bot: I haz the power, 2016-07-13 21:16:26]

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-07-13 21:17:36]

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

[commit-bot: I haz the power, 2016-07-13 21:17:37]

Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  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_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/9013)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[mvstanton, 2016-07-13 21:37:24]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2016-07-13 21:37:24]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2101123005/#ps180001
(title: "rebase changes.")

[commit-bot: I haz the power, 2016-07-13 21:37:31]

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-07-13 21:44:21]

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

[commit-bot: I haz the power, 2016-07-13 21:44:22]

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

[Marija Antic, 2016-07-14 07:36:23]

On 2016/07/13 21:44:22, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

Added the missing files to the mips port.

[mvstanton, 2016-07-14 08:27:18]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2016-07-14 08:27:18]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2101123005/#ps220001
(title: "REBASE.")

[commit-bot: I haz the power, 2016-07-14 08:27:28]

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-07-14 08:53:12]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-07-14 08:53:15]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 08:56:22]

Description was changed from

==========
[turbofan] Introduce integer multiplication with overflow.

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

to

==========
[turbofan] Introduce integer multiplication with overflow.

BUG=

Committed: https://crrev.com/8e18a5f2a0357c6899cd32f5ab66b73d72fdd996
Cr-Commit-Position: refs/heads/master@{#37748}
==========

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

Patchset 12 (id:??) landed as
https://crrev.com/8e18a5f2a0357c6899cd32f5ab66b73d72fdd996
Cr-Commit-Position: refs/heads/master@{#37748}