1136221 - SIMD (interpreter): relax type requirement for the Bool function

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:ProgramFOX]

Currently, Int32x4Bool (which will be called Bool after bug 1124291) in SIMD.cpp requires booleans to be passed as arguments, but args[i] could be any type.

Quoting bbouvier from bug 1124291 comment 4:

> Actually, it seems this requirement is too strong and we could relax it.
> args[i] could be any type (there is no special requirement in the spec
> polyfill, which just applies (!x - 1) to determine what we should store,
> so any value which is !-able could fit in).

Comment 1

[:ProgramFOX]

Attachment: Relax type requirement for Int32x4Bool

Proposed patch for this bug.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8569193 [details] [diff] [review]
Relax type requirement for Int32x4Bool

Review of attachment 8569193 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for jumping in! You can do slightly simpler though...

::: js/src/builtin/SIMD.cpp
@@ +915,5 @@
>      return StoreResult<Vret>(cx, args, result);
>  }
>  
> +static int32_t
> +ToSimdBoolean(HandleValue hv)

There's is a ToBoolean function you can use, which will do the right thing for you. Once you have the boolean, you can convert it into an int (so that it's 1 or 0), and then you can manipulate this.

@@ +933,5 @@
>  static bool
>  Int32x4Bool(JSContext *cx, unsigned argc, Value *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    if (args.length() != 4)

Just figured out that the args.length doesn't need to be harsh like that, we can just generate undefined for missing arguments (see other functions in this file, which should do this). Can you change it as well and add a few tests, please?

::: js/src/tests/ecma_7/SIMD/int32x4bool.js
@@ +41,5 @@
> +  assertEq(e.y, -1);
> +  assertEq(e.z, -1);
> +  assertEq(e.w, -1);
> +
> +  var f = int32x4.bool(undefined, null, undefined, null);

Can you replace one of the undefined by an object literal, please?
Can you also test with an object that can emulate undefined (there's a testing helper for that, iirc)

@@ +47,5 @@
> +  assertEq(f.y, 0);
> +  assertEq(f.z, 0);
> +  assertEq(f.w, 0);
> +
> +  assertThrowsInstanceOf(() => int32x4.bool(abc, def, ghi, jkl), ReferenceError);

Not sure of what this test does?

Comment 3

[:ProgramFOX]

Attachment: Relax type requirement for Int32x4Bool

Updated patch, with the remarks fixed. I wasn't sure what you meant by emulating undefined, so I created an empty variable (`var h = {}`) and called a non-existing property of it (`h.a`), which looked like a sort of emulating to me.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8569222 [details] [diff] [review]
Relax type requirement for Int32x4Bool

Review of attachment 8569222 [details] [diff] [review]:
-----------------------------------------------------------------

Please update the tests and r=me. Thanks for this cleanup!

::: js/src/builtin/SIMD.cpp
@@ +922,4 @@
>  
>      int32_t result[Int32x4::lanes];
>      for (unsigned i = 0; i < Int32x4::lanes; i++)
> +        result[i] = ToBoolean(args.get(i)) ? -1 : 0;

Lovely!

::: js/src/tests/ecma_7/SIMD/int32x4bool.js
@@ +43,5 @@
> +  assertEq(e.w, -1);
> +
> +  var g;
> +  var h = {};
> +  var i = int32x4.bool(undefined, null, g, h.a);

Can you replace g by {}, and h.a by objectEmulatingUndefined(), please?

@@ +53,5 @@
> +  var j = int32x4.bool(true);
> +  assertEq(j.x, -1);
> +  assertEq(j.y, 0);
> +  assertEq(j.z, 0);
> +  assertEq(j.w, 0);

Can you add test cases for the following cases:
- no arguments
- 2 arguments
- 3 arguments
- 5 arguments

Comment 5

[:ProgramFOX]

Attachment: Relax type requirement for Int32x4Bool

Updated patch, with the remarks fixed.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa6ede398040

Comment 6

[:ProgramFOX]

Attachment: Relax type requirement for Int32x4Bool

The try push failed. jonco said on IRC that it might be caused by the fact that objectEmulatingUndefined is only defined in the shell, and J reftests run in the browser [0]. This updated patch defines a tryEmulateUndefined method in SIMD/shell.js: if objectEmulatingUndefined exists, it uses that, if not, it returns undefined.

  [0]:http://logs.glob.uno/?c=mozilla%23jsapi#c550543

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8569962 [details] [diff] [review]
Relax type requirement for Int32x4Bool

Review of attachment 8569962 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the mess, and thanks for updating your patch!

::: js/src/tests/ecma_7/SIMD/shell.js
@@ +99,5 @@
>          assertEq(observed[i], expected[i]);
>  }
> +
> +function tryEmulateUndefined() {
> +    if (objectEmulatingUndefined)

We usually write this kind of test as:

if (typeof objectEmulatingUndefined !== 'undefined')

And feel free to put it in int32x4bool.js directly :)

Comment 8

[:ProgramFOX]

Attachment: Relax type requirement for Int32x4Bool

Updated patch, with the remarks fixed.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83c12fa60447

Comment 9

[:ProgramFOX]

Try push succeeded, adding checkin-needed.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/232e9f16ee88

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/232e9f16ee88