Do edgeCaseAnalysis on ranges for MMul

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: eternalsushant, Assigned: eternalsushant)

Tracking

(Blocks: 1 bug)

unspecified
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 years ago
Bug 998485 for MMul:

canBeNegativeZero is being set based on particular values flowing in during MMul::analyzeEdgeCasesForward.

We should do the same for ranges by adding a function MMul::collectRangeInfoPreTrunc
(Assignee)

Updated

4 years ago
Blocks: 998485
Assignee: nobody → eternalsushant
Whiteboard: [mentor=nbp][lang=c++]
(Assignee)

Comment 1

4 years ago
Created attachment 8440051 [details] [diff] [review]
RangeAnalysis for MMul
Attachment #8440051 - Flags: review?(hv1989)
Thanks! I'm doing the reviews today. Since you now know how to do these, there are only 2 places left. So if you want to do those too? You can open bugs for these yourself too (like you successfully did with this one).

1) MToInt32 has canBeNegativeZero
2) MMod has canBeXXX functions. Can you transform them like MMul/MDiv. E.g. Move the logic of these canBeXXX functions to "analyzeEdgeCasesForward" and add booleans where we store the result? Afterwards it should get the "edgeCaseAnalysis" function doing the same on ranges.
Comment on attachment 8440051 [details] [diff] [review]
RangeAnalysis for MMul

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

::: js/src/jit/MIR.h
@@ +4395,5 @@
>      MDefinition *foldsTo(TempAllocator &alloc, bool useValueNumbers);
>      void analyzeEdgeCasesForward();
>      void analyzeEdgeCasesBackward();
> +    void collectRangeInfoPreTrunc();
> +    

Nit: Remove whitespace.

::: js/src/jit/RangeAnalysis.cpp
@@ +2705,5 @@
> +MMul::collectRangeInfoPreTrunc()
> +{
> +    Range lhsRange(lhs());
> +    Range rhsRange(rhs());
> +    

Nit: remove space.

@@ +2712,5 @@
> +        setCanBeNegativeZero(false);
> +
> +    // Likewise rhsRange.
> +    if (rhsRange.isFiniteNonNegative() && !rhsRange.canBeZero())
> +        setCanBeNegativeZero(false);

Can you also add the following case being canBeNegativeZero(false):
rhs >= 0 && lhs >= 0

And
rhs < 0 && lhs < 0
Attachment #8440051 - Flags: review?(hv1989)
(Assignee)

Comment 4

4 years ago
Created attachment 8440621 [details] [diff] [review]
RangeAnalysis for MMul

Added the 2 cases.
Fixed style nits.
Attachment #8440051 - Attachment is obsolete: true
Attachment #8440621 - Flags: review?(hv1989)
(Assignee)

Comment 5

4 years ago
Created attachment 8440625 [details] [diff] [review]
RangeAnalysis for MMul
Attachment #8440621 - Attachment is obsolete: true
Attachment #8440621 - Flags: review?(hv1989)
Attachment #8440625 - Flags: review?(hv1989)
(Assignee)

Comment 6

4 years ago
Created attachment 8440627 [details] [diff] [review]
RangeAnalysis for MMul
Attachment #8440625 - Attachment is obsolete: true
Attachment #8440625 - Flags: review?(hv1989)
Attachment #8440627 - Flags: review?(hv1989)
(Assignee)

Comment 7

4 years ago
Created attachment 8440634 [details] [diff] [review]
RangeAnalysis for MMul
Attachment #8440627 - Attachment is obsolete: true
Attachment #8440627 - Flags: review?(hv1989)
Attachment #8440634 - Flags: review?(hv1989)
Attachment #8440634 - Attachment is patch: true
Attachment #8440634 - Flags: review?(hv1989) → review+
Created attachment 8440642 [details] [diff] [review]
RangeAnalysis for MMul

(was formatted in windows newlines)
Attachment #8440634 - Attachment is obsolete: true
Mentor: nicolas.b.pierron
Whiteboard: [mentor=nbp][lang=c++] → [lang=c++]
https://hg.mozilla.org/mozilla-central/rev/1a1060ba8672
Mentor: nicolas.b.pierron
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Mentor: nicolas.b.pierron
Mentor: nicolas.b.pierron
You need to log in before you can comment on or make changes to this bug.