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

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ProgramFOX, Assigned: ProgramFOX)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

3 years ago
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).
(Assignee)

Comment 1

3 years ago
Created attachment 8569193 [details] [diff] [review]
Relax type requirement for Int32x4Bool

Proposed patch for this bug.
Attachment #8569193 - Flags: review?(benj)
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?
Attachment #8569193 - Flags: review?(benj) → feedback+
(Assignee)

Comment 3

3 years ago
Created attachment 8569222 [details] [diff] [review]
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.
Attachment #8569193 - Attachment is obsolete: true
Attachment #8569222 - Flags: review?(benj)
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
Attachment #8569222 - Flags: review?(benj) → review+
(Assignee)

Comment 5

3 years ago
Created attachment 8569273 [details] [diff] [review]
Relax type requirement for Int32x4Bool

Updated patch, with the remarks fixed.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa6ede398040
Assignee: nobody → programfox
Attachment #8569222 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8569273 - Flags: review+
(Assignee)

Comment 6

3 years ago
Created attachment 8569962 [details] [diff] [review]
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
Attachment #8569273 - Attachment is obsolete: true
Attachment #8569962 - Flags: review?(benj)
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 :)
Attachment #8569962 - Flags: review?(benj) → review+
(Assignee)

Comment 8

3 years ago
Created attachment 8570534 [details] [diff] [review]
Relax type requirement for Int32x4Bool

Updated patch, with the remarks fixed.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83c12fa60447
Attachment #8569962 - Attachment is obsolete: true
Attachment #8570534 - Flags: review+
(Assignee)

Comment 9

3 years ago
Try push succeeded, adding checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/232e9f16ee88
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.