[strong] Disallow implicit conversions for bitwise ops, shifts

[strong] Disallow implicit conversions for bitwise ops, shifts

See https://codereview.chromium.org/1092353002/

Due to parser rewrites, also implements restrictions for unary ~.

Still to come, implementing restrictions for binary + and comparison.

BUG=v8:3956
LOG=N

Committed: https://crrev.com/6988aec61f071a1caec96abae1cf39353a234455
Cr-Commit-Position: refs/heads/master@{#28104}

[conradw, 2015-04-24 14:52:58]

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

[conradw, 2015-04-24 14:53:18]

PTAL

[arv (Not doing code reviews), 2015-04-24 15:39:43]

No cctests?

Your CL descriptions mentions unary + but not unary -. Fine to split them up as
you wish.

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

https://codereview.chromium.org/1102923002/diff/60001/src/compiler/js-typed-l...
src/compiler/js-typed-lowering.cc:362: if (r.BothInputsAre(Type::Number())) {
Let me make sure I understand this. If the operands are not numbers we keep the
nodes which will cause the runtime call which will throw?

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

https://codereview.chromium.org/1102923002/diff/60001/test/mjsunit/strong/imp...
test/mjsunit/strong/implicit-conversions.js:10: let strong_binops = [
FYI. Our js style does not allow underscores in variable names. I know the V8
team has historically been opposed to follow the google style guide though...

https://codereview.chromium.org/1102923002/diff/60001/test/mjsunit/strong/imp...
test/mjsunit/strong/implicit-conversions.js:108: function not_strong(x, y) {
Not clear why you need this function. You are testing the unary ops later.

[conradw, 2015-04-27 11:20:47]

The + in the CL description was meant to refer to binary +,
instead of unary +. I've clarified. It turns out unary +,-
are gotten for free via parser re-writes, so I've added tests
for them. cctests from the last CL should already be testing
the strong versions of these ops, I was lazy last time
updating a few unittests, I fixed them so they now test
their strong mode versions.

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

https://codereview.chromium.org/1102923002/diff/60001/src/compiler/js-typed-l...
src/compiler/js-typed-lowering.cc:362: if (r.BothInputsAre(Type::Number())) {
On 2015/04/24 15:39:42, arv wrote:
> Let me make sure I understand this. If the operands are not numbers we keep
the
> nodes which will cause the runtime call which will throw?

Yes. Without this case the Binop call could be lowered while inputs are
not guaranteed to be numbers (since convertInputsToNumber is used), which
would effectively replace the strong mode version with the sloppy mode
version. I made a mistake in the shift version of this, which I've fixed
in the next patchset.

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

https://codereview.chromium.org/1102923002/diff/60001/test/mjsunit/strong/imp...
test/mjsunit/strong/implicit-conversions.js:10: let strong_binops = [
On 2015/04/24 15:39:42, arv wrote:
> FYI. Our js style does not allow underscores in variable names. I know the V8
> team has historically been opposed to follow the google style guide though...

Hah! I should at least try and be consistent, since I changed this elsewhere.

https://codereview.chromium.org/1102923002/diff/60001/test/mjsunit/strong/imp...
test/mjsunit/strong/implicit-conversions.js:108: function not_strong(x, y) {
On 2015/04/24 15:39:42, arv wrote:
> Not clear why you need this function. You are testing the unary ops later.

Yeah, it was a mistake to put this here.

[conradw, 2015-04-27 11:21:37]

conradw@chromium.org changed reviewers:
+ dslomov@chromium.org

[arv (Not doing code reviews), 2015-04-27 14:02:49]

LGTM

Do you want to add tests for *=, /= etc?

https://codereview.chromium.org/1102923002/diff/140001/test/mjsunit/strong/im...
File test/mjsunit/strong/implicit-conversions.js (right):

https://codereview.chromium.org/1102923002/diff/140001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:7: 'use strict';
Use double quotes here too for consistency.

[titzer, 2015-04-27 14:06:46]

On 2015/04/27 14:02:49, arv wrote:
> LGTM
> 
> Do you want to add tests for *=, /= etc?
> 
>
https://codereview.chromium.org/1102923002/diff/140001/test/mjsunit/strong/im...
> File test/mjsunit/strong/implicit-conversions.js (right):
> 
>
https://codereview.chromium.org/1102923002/diff/140001/test/mjsunit/strong/im...
> test/mjsunit/strong/implicit-conversions.js:7: 'use strict';
> Use double quotes here too for consistency.

Might want to have mstarzinger@ have a look at CLs that touch TF internals.

[rossberg, 2015-04-27 14:32:19]

On 2015/04/27 14:02:49, arv wrote:
> Do you want to add tests for *=, /= etc?

+1. Also ++ and --.

[conradw, 2015-04-27 20:35:15]

On 2015/04/27 14:32:19, rossberg wrote:
> On 2015/04/27 14:02:49, arv wrote:
> > Do you want to add tests for *=, /= etc?
> 
> +1. Also ++ and --.

I fell down a rabbit hole today trying to discover how to make ++ and -- work.
It appears that in the platform specific assembly (full codegen), an inlined
call to ToNumber on the argument is inserted wherever a ++ or -- occurs, before
the stub is invoked (if at all). This behaviour is also duplicated across
various other bits of optimising code, so getting the change right might be
horrible, and definitely deserves its own CL. I'm going to finish the other ops
first before going after it.

[conradw, 2015-04-27 20:41:56]

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

[conradw, 2015-04-27 20:41:57]

Added the extra test cases, bearing in mind my comment about ++,--

@mstarzinger, are the TF changes ok?

https://codereview.chromium.org/1102923002/diff/140001/test/mjsunit/strong/im...
File test/mjsunit/strong/implicit-conversions.js (right):

https://codereview.chromium.org/1102923002/diff/140001/test/mjsunit/strong/im...
test/mjsunit/strong/implicit-conversions.js:7: 'use strict';
On 2015/04/27 14:02:49, arv wrote:
> Use double quotes here too for consistency.

Done.

[rossberg, 2015-04-27 21:17:17]

lgtm

[Michael Starzinger, 2015-04-28 11:07:33]

The TurboFan changes LGTM, no comment on the rest.

[conradw, 2015-04-28 11:10:23]

The CQ bit was checked by conradw@chromium.org

[conradw, 2015-04-28 11:10:23]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1102923002/#ps160001
(title: "cl feedback 2")

[commit-bot: I haz the power, 2015-04-28 11:10:29]

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

[commit-bot: I haz the power, 2015-04-28 11:20:17]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-04-28 11:20:34]

Patchset 9 (id:??) landed as
https://crrev.com/6988aec61f071a1caec96abae1cf39353a234455
Cr-Commit-Position: refs/heads/master@{#28104}