Closed Bug 681745 Opened 13 years ago Closed 13 years ago

IonMonkey: Implement MTableSwitch(MIRType_Double)

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(1 file)

This is needed for MTableSwitch.
Summary: IonMonkey: Implement MToInt32(MIRType_Double) → IonMonkey: Implement MTableSwitch(MIRType_Double)
Attached patch fixSplinter Review
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)
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+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.