Closed Bug 1136837 Opened 5 years ago Closed 5 years ago
Fix some inlining issues
I just noticed a few issues on Octane-TypeScript: (1) We're inlining functions even if |this| or one of the arguments has an empty resultTypeSet. We probably shouldn't do that because it means the call will never be reached (without recompiling the script). Inlining in this case *could* help alias analysis, but I really doubt it's worth the extra compile time. (2) When we have: if (o && o.foo()) Inside foo, we may not know that |this| is an object (it could have an object-or-null/undefined TypeSet). If o is null/undefined, o.foo would throw an exception though so |this| must be an object. Currently we add MComputeThis, which has no resultTypeSet, so using |this| is much slower than necessary.
This fixes the first problem in comment 0. This keeps us from inlining a lot of scripts on TypeScript (though it's possible they'd hit other aborts later) and we win a few hundred points locally (but noisy). The difference is a bit bigger and more reliable with --no-threads, which makes sense. The inlining abort also hits on other benchmarks like deltablue, pdfjs, box2d and mandreel, but I see no difference in our score there.
Attachment #8569699 - Flags: review?(hv1989)
Comment on attachment 8569699 [details] [diff] [review] Part 1 - Don't inline if argument types are incomplete Review of attachment 8569699 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: js/public/TrackedOptimizationInfo.h @@ +203,5 @@ > "can't inline: not hot enough") \ > _(CantInlineNotInDispatch, \ > "can't inline: not in dispatch table") \ > + _(CantInlineUnreachable, \ > + "can't inline: unreachable due to incomplete this/argument types")\ s/argument/arguments ::: js/src/jit/IonBuilder.cpp @@ +4691,5 @@ > + trackOptimizationOutcome(TrackedOutcome::CantInlineUnreachable); > + return InliningDecision_DontInline; > + } > + } > + } Can you move this into "canInlineTarget"? I think non heuristic callee checks should go in there.
Attachment #8569699 - Flags: review?(hv1989) → review+
This fixes the second problem and another minor related issue that came up on TypeScript.
Attachment #8569729 - Flags: review?(hv1989)
Thanks for the quick review. (In reply to Hannes Verschore [:h4writer] from comment #2) > > + _(CantInlineUnreachable, \ > > + "can't inline: unreachable due to incomplete this/argument types")\ > > s/argument/arguments "incomplete arguments types" is not correct English, AFAIK, but I can rephrase it as "incomplete types for this/arguments". > Can you move this into "canInlineTarget"? Sure.
Attachment #8569729 - Flags: review?(hv1989) → review+
You need to log in before you can comment on or make changes to this bug.