Closed Bug 1075967 Opened 10 years ago Closed 10 years ago

Inline ToInteger, if TypeSet contains multiple optimizable types

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jschulte, Assigned: jschulte)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch v1.patch (obsolete) — Splinter Review
See bug 1059426 comment 8 .

I hope, I covered all types, which bail out.
Attachment #8498403 - Flags: review?(hv1989)
Comment on attachment 8498403 [details] [diff] [review]
v1.patch

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

Thanks for this follow-up bug!

Note: you don't have to take the resultTypeSet to test. You can use something much easier/better:
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#580
so
callInfo.getArg(0)->mightBeType(MIRType_Undefined);

Also if you want to improve it a little bit more:
1) Create IsMagicType in IonTypes.h:
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonTypes.h?from=IonTypes.h&case=true#506
This would return true for any MIRType_MagicXXX we currently have.

2) Create mightBeMagicType() in MIR.h
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#580
Which is a copy of mightBeType, but 
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#583
changed to IsMagicType(type())
and
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#589
to the magic testing you had
types->hasType(types::Type::MagicArgType())

That way you can use mightByMagicType() for the Magic testing.

::: js/src/jit/MCallOptimize.cpp
@@ +2067,2 @@
>  
> +    if(!types)

Style nit: please add a space between "if" and "("

@@ +2069,3 @@
>          return InliningStatus_NotInlined;
>  
> +    // Only optimize cases where input is number, null or boolean

Can you update comment to:
// Only optimize cases where input contains only number, null or boolean.

@@ +2071,5 @@
> +    // Only optimize cases where input is number, null or boolean
> +    if (types->maybeObject() || types->hasType(types::Type::UndefinedType()) ||
> +        types->hasType(types::Type::StringType()) || types->hasType(types::Type::SymbolType()) ||
> +        types->hasType(types::Type::MagicArgType()))
> +            return InliningStatus_NotInlined;

Style nit: If the condition is longer than one line we use {}
Like:

> if (foo ||
>     bar)
> {
> 
> }

Also a preference, but could you have a newline after each || for this. That will make it easier readable, since everything will be aligned.

@@ +2072,5 @@
> +    if (types->maybeObject() || types->hasType(types::Type::UndefinedType()) ||
> +        types->hasType(types::Type::StringType()) || types->hasType(types::Type::SymbolType()) ||
> +        types->hasType(types::Type::MagicArgType()))
> +            return InliningStatus_NotInlined;
> +

Can you add here:
MOZ_ASSERT(IsNumberType(type) || type == MIRType_Null || type == MIRType_Boolean || type == MIRType_Value);
Attachment #8498403 - Flags: review?(hv1989)
Attached patch v2.patchSplinter Review
Thanks for the quick review!

Yes, the number of (Magic-)MIRTypes made me switch to Type, but this way its definitely nicer :)
Attachment #8498403 - Attachment is obsolete: true
Attachment #8499725 - Flags: review?(hv1989)
Comment on attachment 8499725 [details] [diff] [review]
v2.patch

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

Thanks, this looks good!
Attachment #8499725 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/a799b8e48161
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.