Closed Bug 1024895 Opened 10 years ago Closed 10 years ago

IonMonkey: Implement Floor Recover Instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bbouvier, Assigned: tandiap, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 3 obsolete files)

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.
Mentor: benj
Whiteboard: [good first bug][mentor=bbouvier][lang=c++] → [good first bug][lang=c++]
Working on it.
Assignee: nobody → exitmrfu+bugzilla
Mentor: benj → nicolas.b.pierron
Attached patch Floor recover instructions (obsolete) — Splinter Review
Attachment #8444055 - Flags: review?(nicolas.b.pierron)
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/
Attachment #8444055 - Flags: review?(nicolas.b.pierron) → feedback+
Attached patch 1024895.patch (obsolete) — Splinter Review
Attachment #8444055 - Attachment is obsolete: true
Attachment #8444061 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8444061 [details] [diff] [review]
1024895.patch

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

Reindent your patch for the next iteration ;)
Attachment #8444061 - Flags: review?(nicolas.b.pierron)
Attached patch 1024895.patch (obsolete) — Splinter Review
Attachment #8444061 - Attachment is obsolete: true
Attached patch 1024895.patchSplinter Review
Attachment #8444078 - Attachment is obsolete: true
Attachment #8444089 - Flags: review?(nicolas.b.pierron)
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)
Attachment #8444089 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8444089 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/ec5b6129445b
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.