If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

IonMonkey: Implement MTableSwitch(MIRType_Double)

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
This is needed for MTableSwitch.
(Assignee)

Updated

6 years ago
Summary: IonMonkey: Implement MToInt32(MIRType_Double) → IonMonkey: Implement MTableSwitch(MIRType_Double)
(Assignee)

Comment 1

6 years ago
Created attachment 555575 [details] [diff] [review]
fix

Most of this patch is refactoring of bailoutIf, so emitDoubleToInt32 can be used for both deopts and safe failures (like for table switch).

I could go back to the LDoubleToInt32 solution, which would (unnecessarily) deoptimize if switch encountered a double, if that seemed simpler, but I can't yet see if it would have any other consumers.
Attachment #555575 - Flags: review?(sstangl)
(Assignee)

Comment 2

6 years ago
Comment on attachment 555575 [details] [diff] [review]
fix

Hannes, could you look at the table switch changes?
 * It now uses the COPY policy when it can.
 * Took out the LIR-level constant folding - mostly because it was about to
   get more complicated, so maybe if it ends up being important we should look
   at ways to fold constant branches in MIR. But I can add it back if you want.
Attachment #555575 - Flags: review?(hv1989)
Comment on attachment 555575 [details] [diff] [review]
fix

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

Seems all right.

About the constant folding:
- I think constant in a switch won't occur that much. I only see people that are testing doing that. So it's not that big of a deal.
- From what I see the code wouldn't get that much more complicated. (Just a double(int(index)) == index_d check if type is double)
- Adding it to MIR would work but would be very very hacky!
Attachment #555575 - Flags: review?(hv1989) → review+
Comment on attachment 555575 [details] [diff] [review]
fix

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

This seems fine. I am unclear on how this logic would prettily transfer to permit handling switches with double-valued cases, but I suspect it is not a meaningful issue.

::: js/src/ion/TypePolicy.cpp
@@ +270,5 @@
>      MDefinition *in = ins->getOperand(0);
>      MInstruction *replace;
>  
>      // Tableswitch can consume all types, except:
>      // - Double: try to convert to int32

Can remove this comment now.

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +463,5 @@
>  
> +    if (ins->index()->isDouble()) {
> +        temp = ins->tempInt();
> +
> +        // The input is a double, so try and convert it to an integer. If it

"If it" to line below.

@@ +464,5 @@
> +    if (ins->index()->isDouble()) {
> +        temp = ins->tempInt();
> +
> +        // The input is a double, so try and convert it to an integer. If it
> +        // does not fit in an integer, take the default case.

Note that this particular implementation of TableSwitch is limited to only those switches with integer-valued cases.
Attachment #555575 - Flags: review?(sstangl) → review+
(Assignee)

Comment 5

6 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/a4e9fd62263d and another checkin for follow-up nits
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.