1024895 - IonMonkey: Implement Floor Recover Instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Implement RFloor in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.

MFloor is the operation triggered by Math.floor(double) -> int32.

Comment 1

[Paali Tandia]

Working on it.

Comment 2

[Paali Tandia]

Attachment: Floor recover instructions

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444055 [details] [diff] [review]
Floor recover instructions

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

Ok, this looks good. :)
Still a few fix before being able to land this patch ;)

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +281,5 @@
>  }
>  
> +var uceFault_floor_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_floor_number'));
> +function rfloor_number(i) {
> +    var x = Math.floor(99.1111);

use "i + 0.1", to ensure that this test will not become invalid due to constant propagation.

::: js/src/jit/Recover.cpp
@@ +509,5 @@
> +{
> +  RootedValue v(cx, iter.read());
> +  RootedValue result(cx);
> +
> +  if (!js::math_floor_hv(cx, v, &result)) 

style-nit: remove trailing whitespace.

@@ +513,5 @@
> +  if (!js::math_floor_hv(cx, v, &result)) 
> +    return false;
> +
> +  iter.storeInstructionResult(result);
> +  return true;

style-nit: We use 4 spaces indents.

::: js/src/jit/Recover.h
@@ +29,5 @@
>      _(Sub)                                      \
>      _(Mul)                                      \
>      _(Div)                                      \
>      _(Mod)                                      \
> +    _(Floor)                                    \

In all files, follow the same order of the list defined in Bug 1003801.

::: js/src/jsmath.cpp
@@ +424,5 @@
>      return floor(x);
>  }
>  
>  bool
> +js::math_floor_hv(JSContext *cx, HandleValue v, MutableHandleValue r)

nit: s/math_floor_hv/math_floor_handle/

Comment 4

[Paali Tandia]

Attachment: 1024895.patch

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444061 [details] [diff] [review]
1024895.patch

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

Reindent your patch for the next iteration ;)

Comment 6

[Paali Tandia]

Attachment: 1024895.patch

Comment 7

[Paali Tandia]

Attachment: 1024895.patch

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444089 [details] [diff] [review]
1024895.patch

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

::: js/src/jit/Recover.cpp
@@ +523,5 @@
> +bool RFloor::recover(JSContext *cx, SnapshotIterator &iter) const
> +{
> +    RootedValue v(cx, iter.read());
> +    RootedValue result(cx);
> +    

style-nit: Remove trailing whitespace.

::: js/src/jit/Recover.h
@@ +271,5 @@
>  
> +class RFloor MOZ_FINAL : public RInstruction
> +{
> +    public:
> +        RINSTRUCTION_HEADER_(Floor)

style-nit: Fix indentation:

␣␣public:
␣␣␣␣RINSTRUCTION_HEADER_(Floor)

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8444089 [details] [diff] [review]
1024895.patch

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

I will fix the following modification, and land the patch.

Comment 10

[Nicolas B. Pierron [:nbp]]

https://tbpl.mozilla.org/?tree=Try&showall=1&rev=b9a4bebbe04d

Comment 11

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec5b6129445b

I've landed your patch as part of the contribution of others.  You will find the result of it on our continuous integration system (tbpl) [1].  Later a Sheriff will take the patches and merge into mozilla-central.


[1] https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ec5b6129445b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec5b6129445b