1265748 - non-Object path in IonBuilder::inlineIsCallable is not used.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Tooru Fujisawa [:arai]]

> IonBuilder::InliningStatus
> IonBuilder::inlineIsCallable(CallInfo& callInfo)
> {
> ...
>     if (callInfo.getArg(0)->type() != MIRType_Object)
>         return InliningStatus_NotInlined;
> 
>     // Try inlining with constant true/false: only objects may be callable at
>     // all, and if we know the class check if it is callable.
>     bool isCallableKnown = false;
>     bool isCallableConstant;
>     if (callInfo.getArg(0)->type() != MIRType_Object) {
>         isCallableKnown = true;
>         isCallableConstant = false;
>     } else {
>     ...
> }

2nd |callInfo.getArg(0)->type() != MIRType_Object| check won't become true.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Enable non-Object path in IonBuilder::inlineIsCallable.

IsCallable should be known to be false if the argument is primitive (including undefined and null).
It's not inlined if |type() > MIRType_Object|, like if MIRType_Value.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3115a732376f

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8743110 [details] [diff] [review]
Enable non-Object path in IonBuilder::inlineIsCallable.

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

Wow that logic was broken. Thanks for the patch, and nice test.

::: js/src/jit/MCallOptimize.cpp
@@ +2661,5 @@
>      if (getInlineReturnType() != MIRType_Boolean)
>          return InliningStatus_NotInlined;
> +
> +    MDefinition* arg = callInfo.getArg(0);
> +    if (arg->type() > MIRType_Object)

Could use a comment on what it means for the type to be > MIRType_Object.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8743110 [details] [diff] [review]
Enable non-Object path in IonBuilder::inlineIsCallable.

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

::: js/src/jit/MCallOptimize.cpp
@@ +2661,5 @@
>      if (getInlineReturnType() != MIRType_Boolean)
>          return InliningStatus_NotInlined;
> +
> +    MDefinition* arg = callInfo.getArg(0);
> +    if (arg->type() > MIRType_Object)

Also make a similar comment change in IonTypes.h, unless enum MIRType on trunk doesn't have such a comment.  (And I'd probably put the comment inline in the enum initializer list, rather than [potentially] out of context atop the enum.)

Comment 4

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b609cab107795bd79520e1674ddfdee80e46a8a
Bug 1265748 - Enable non-Object path in IonBuilder::inlineIsCallable. r=shu

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0b609cab1077