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)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: jschulte, Assigned: jschulte)
References
Details
Attachments
(1 file, 1 obsolete file)
3.62 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
See bug 1059426 comment 8 . I hope, I covered all types, which bail out.
Attachment #8498403 -
Flags: review?(hv1989)
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ccd13fd71ccf
Keywords: checkin-needed
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a799b8e48161
Keywords: checkin-needed
Comment 6•10 years ago
|
||
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.
Description
•