[strong] Disallow implicit conversions for add

[strong] Disallow implicit conversions for add

Implements the strong mode proposal's restrictions on implicit conversions
for the binary + operator. Test suite is also cleaned up/refactored to allow
easier testing of the comparison operators in the future.

BUG=v8:3956
LOG=N

Committed: https://crrev.com/dea0d9b59e25287ffc1fd5eec29cbe0a71317864
Cr-Commit-Position: refs/heads/master@{#28159}

[conradw, 2015-04-29 15:32:49]

conradw@chromium.org changed reviewers:
+ arv@chromium.org, dslomov@chromium.org, mstarzinger@chromium.org,
rossberg@chromium.org

[conradw, 2015-04-29 15:32:49]

PTAL

[Dmitry Lomov (no reviews), 2015-04-30 08:43:59]

lgtm, just a minor comment

https://codereview.chromium.org/1109223004/diff/1/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1109223004/diff/1/src/runtime.js#newcode163
src/runtime.js:163: //ECMA-262, section 11.6.1, page 50.
This comment does not apply anymore I guess.

[conradw, 2015-04-30 09:19:33]

https://codereview.chromium.org/1109223004/diff/1/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1109223004/diff/1/src/runtime.js#newcode163
src/runtime.js:163: //ECMA-262, section 11.6.1, page 50.
On 2015/04/30 08:43:59, Dmitry Lomov (chromium) wrote:
> This comment does not apply anymore I guess.

Changed this and similar examples in other places.

[Michael Starzinger, 2015-04-30 09:25:56]

As discussed offline: We might consider an approach where ToNumber and ToString
operations are still inserted, but have a throwing behavior in strong mode.

I am fine with landing this CL as it is and follow-up on this later if we decide
to adapt our approach here.

https://codereview.chromium.org/1109223004/diff/20001/src/compiler/js-typed-l...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1109223004/diff/20001/src/compiler/js-typed-l...
src/compiler/js-typed-lowering.cc:324:
!is_strong(OpParameter<LanguageMode>(node))) {
nit: Please add JSBinopReduction::IsStrong to have a single choke point for the
scary OpParameter cast. By now there are 5 instances of this, so having this
helper is warranted.

[conradw, 2015-04-30 09:46:04]

https://codereview.chromium.org/1109223004/diff/20001/src/compiler/js-typed-l...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1109223004/diff/20001/src/compiler/js-typed-l...
src/compiler/js-typed-lowering.cc:324:
!is_strong(OpParameter<LanguageMode>(node))) {
On 2015/04/30 09:25:56, Michael Starzinger wrote:
> nit: Please add JSBinopReduction::IsStrong to have a single choke point for
the
> scary OpParameter cast. By now there are 5 instances of this, so having this
> helper is warranted.

Done.

[Michael Starzinger, 2015-04-30 10:19:21]

LGTM on the TurboFan part.

https://codereview.chromium.org/1109223004/diff/40001/src/compiler/js-typed-l...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1109223004/diff/40001/src/compiler/js-typed-l...
src/compiler/js-typed-lowering.cc:336: if (r.OneInputIs(Type::String()) &&
(!r.IsStrong() ||
suggestion: Would the following be easier to read/understand?

if ((r.OneInputIs(Type::String() && !r.IsStrong()) ||
    r.BothInputsAre(Type::String())) {
...
}

[conradw, 2015-04-30 10:27:34]

https://codereview.chromium.org/1109223004/diff/40001/src/compiler/js-typed-l...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1109223004/diff/40001/src/compiler/js-typed-l...
src/compiler/js-typed-lowering.cc:336: if (r.OneInputIs(Type::String()) &&
(!r.IsStrong() ||
On 2015/04/30 10:19:21, Michael Starzinger wrote:
> suggestion: Would the following be easier to read/understand?
> 
> if ((r.OneInputIs(Type::String() && !r.IsStrong()) ||
>     r.BothInputsAre(Type::String())) {
> ...
> }

Ah, that's much nicer.

[conradw, 2015-04-30 11:38:19]

The CQ bit was checked by conradw@chromium.org

[conradw, 2015-04-30 11:38:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
dslomov@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1109223004/#ps60001
(title: "cl feedback 3")

[commit-bot: I haz the power, 2015-04-30 11:38:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1109223004/60001

[commit-bot: I haz the power, 2015-04-30 11:46:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-30 11:46:22]

Patchset 4 (id:??) landed as
https://crrev.com/dea0d9b59e25287ffc1fd5eec29cbe0a71317864
Cr-Commit-Position: refs/heads/master@{#28159}

[arv (Not doing code reviews), 2015-04-30 16:07:32]

LGTM

I would also like to see some tests with template literals.

https://codereview.chromium.org/1109223004/diff/60001/test/mjsunit/strong/imp...
File test/mjsunit/strong/implicit-conversions.js (right):

https://codereview.chromium.org/1109223004/diff/60001/test/mjsunit/strong/imp...
test/mjsunit/strong/implicit-conversions.js:80: function add_num_strong(x, y) {
Any reason for this over add_strong?

https://codereview.chromium.org/1109223004/diff/60001/test/mjsunit/strong/imp...
test/mjsunit/strong/implicit-conversions.js:294: assertThrows(function(){func(2,
"foo");}, TypeError);
Shouldn't this use a non smi?

[conradw, 2015-05-04 09:28:22]

https://codereview.chromium.org/1109223004/diff/60001/test/mjsunit/strong/imp...
File test/mjsunit/strong/implicit-conversions.js (right):

https://codereview.chromium.org/1109223004/diff/60001/test/mjsunit/strong/imp...
test/mjsunit/strong/implicit-conversions.js:80: function add_num_strong(x, y) {
On 2015/04/30 16:07:32, arv wrote:
> Any reason for this over add_strong?

It's a kind of unfortunate way to make sure that the type feedback is starting
over from scratch (the two functions are used in different tests).

There's no way to clear type feedback on the fly for binop ICs using a natives
function. %ClearFunctionTypeFeedback wasn't doing what I thought it was.

https://codereview.chromium.org/1109223004/diff/60001/test/mjsunit/strong/imp...
test/mjsunit/strong/implicit-conversions.js:294: assertThrows(function(){func(2,
"foo");}, TypeError);
On 2015/04/30 16:07:32, arv wrote:
> Shouldn't this use a non smi?

I'm not particularly worried in this case, because of the second argument it's
going to be a miss either way.