Closed Bug 1027510 Opened 8 years ago Closed 8 years ago

Ionmonkey: Add Range analysis for Math.Ceil

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: eternalsushant, Assigned: eternalsushant, Mentored)

Details

Attachments

(1 file, 2 obsolete files)

Make a computeRange function for MCeil.
Summary: Ionmonkey: Add Range analysis for Math.ceil → Ionmonkey: Add Range analysis for Math.Ceil
Assignee: nobody → eternalsushant
Mentor: nicolas.b.pierron
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch RangeAnalysis for MCeil (obsolete) — Splinter Review
Attachment #8443519 - Flags: review?(hv1989)
Comment on attachment 8443519 [details] [diff] [review]
RangeAnalysis for MCeil

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

::: js/src/jit/RangeAnalysis.cpp
@@ +1188,5 @@
> +MCeil::computeRange(TempAllocator &alloc)
> +{
> +    Range other(input());
> +    Range *copy = new(alloc) Range(other);
> +    copy->resetFractionalPart();

This is not correct, because we need to extend the upper bound in case where we have a fractional part.
Comment on attachment 8443519 [details] [diff] [review]
RangeAnalysis for MCeil

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

::: js/src/jit/RangeAnalysis.cpp
@@ +1188,5 @@
> +MCeil::computeRange(TempAllocator &alloc)
> +{
> +    Range other(input());
> +    Range *copy = new(alloc) Range(other);
> +    copy->resetFractionalPart();

Has Hannes made me remark, we already include the fractional part in the ranges.
1.5 is represented as [1..2] with fractional part.

So reset fractional part is correct for the Int32 range.
But the way we compute the exponent does not work the same way as we compute it with as the exponent of the number in Double representation.
Attached patch RangeAnalysis for MCeil (obsolete) — Splinter Review
Attachment #8443519 - Attachment is obsolete: true
Attachment #8443519 - Flags: review?(hv1989)
Attachment #8444328 - Flags: review?(hv1989)
Comment on attachment 8444328 [details] [diff] [review]
RangeAnalysis for MCeil

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

Looks good. Can you also add a patch that removes "Decrement lower bound" of floor? Since we discussed it isn't needed for ceil. It also isn't for floor.

Note: since RA is so hard, we decided that for RA changes patches need 2 reviews. Flagging nbp.

::: js/src/jit/RangeAnalysis.cpp
@@ +979,5 @@
>      return copy;
>  }
>  
> +Range *
> +Range::mceil(TempAllocator &alloc, const Range *op)

s/mceil/ceil/

@@ +986,5 @@
> +
> +    if (copy->hasInt32Bounds())
> +        copy->max_exponent_ = copy->exponentImpliedByInt32Bounds();
> +    else if (copy->max_exponent_ < MaxFiniteExponent)
> +        copy->max_exponent_++;

Can you add a comment why we need to increase the exponent?
Attachment #8444328 - Flags: review?(nicolas.b.pierron)
Attachment #8444328 - Flags: review?(hv1989)
Attachment #8444328 - Flags: review+
Comment on attachment 8444328 [details] [diff] [review]
RangeAnalysis for MCeil

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

::: js/src/jit/RangeAnalysis.cpp
@@ +986,5 @@
> +
> +    if (copy->hasInt32Bounds())
> +        copy->max_exponent_ = copy->exponentImpliedByInt32Bounds();
> +    else if (copy->max_exponent_ < MaxFiniteExponent)
> +        copy->max_exponent_++;

In the comment that Hannes mentionned, also mention why we can restrict to MaxTruncatableExponent and not increment the value if it is above 2 ** MaxTruncatableExponent.

@@ +1225,5 @@
>  void
> +MCeil::computeRange(TempAllocator &alloc)
> +{
> +    Range other(getOperand(0));
> +    setRange(Range::mceil(alloc,&other));

style-nit: add a space after the comma.
Attachment #8444328 - Flags: review?(nicolas.b.pierron) → review+
I'm waiting for an updated patch, before I can get this in the tree.
Flags: needinfo?(eternalsushant)
Sorry for the delay.
Attachment #8444328 - Attachment is obsolete: true
Attachment #8447267 - Flags: review?(hv1989)
Flags: needinfo?(eternalsushant)
Attachment #8447267 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/f17234995f03
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.