The default bug view has changed. See this FAQ.

IonMonkey: Implement JSOP_TYPEOF

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
+++ This bug was initially created as a clone of Bug #685838 +++

Implement the opcode  JSOP_TYPEOF in ionmonkey.
(Assignee)

Updated

6 years ago
Assignee: general → evilpies
No longer depends on: 691340
(Assignee)

Comment 1

6 years ago
Created attachment 564639 [details] [diff] [review]
WIP v0

Draft, isn't very useful besides function () { var a = 1; return typeof a; }.
Going to work on this when C++ calls landed (or when i figured out what patches i need). Also we should implement TypeOfIs for cases like typeof x == 'function'.

Apparently there is no jsop_eq either.
(Assignee)

Comment 2

5 years ago
Created attachment 580414 [details] [diff] [review]
WIP v1

It actually works on x64 :O. I still need to figure out if I could implement TypeOfValue in x86-shared. And maybe I shouldn't actually mess with typeAtoms and just create an array.
Attachment #564639 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Created attachment 582824 [details] [diff] [review]
v1

This patch is probably not in the best state, but I want to get this going now.
If we had ToValue in CodeGeneratorX86Shared, we could be able to move visitTypeOfValue there.

Btw: since the first iteration, the callVM architecture improved a lot, good work
Attachment #580414 - Attachment is obsolete: true
Attachment #582824 - Flags: review?(dvander)

Comment 4

5 years ago
I've read your patch and may have an suggestion for LTypeofObject. It is possible without calling functions:

bool
CodeGeneratorX86Shared::visitTypeofO(LTypeofO *lir)
{
    JSContext *cx = GetIonContext()->cx;
    
    Label func;
    Label xml;
    Label object;
    Label end;

    Register input = ToRegister(lir->input());
    Register output = ToRegister(lir->output());

    masm.loadBaseShape(input, output);
    masm.cmpPtr(Operand(output, BaseShape::offsetOfClass()), ImmWord(&js::FunctionClass));
    masm.j(Assembler::Equal, &func);
    masm.cmpPtr(Operand(output, BaseShape::offsetOfClass()), ImmWord(&js::XMLClass));
    masm.j(Assembler::Equal, &xml);
    
    // object
    JSString* objectType = cx->runtime->atomState.typeAtoms[JSTYPE_OBJECT];
    masm.movePtr(ImmWord(objectType), output);
    masm.jmp(&end);

    // function
    JSString* functionType = cx->runtime->atomState.typeAtoms[JSTYPE_FUNCTION];
    masm.bind(&func);
    masm.movePtr(ImmWord(functionType), output);
    masm.jmp(&end);
    
    // xml
    JSString* xmlType = cx->runtime->atomState.typeAtoms[JSTYPE_XML];
    masm.bind(&xml);
    masm.movePtr(ImmWord(xmlType), output);

    masm.bind(&end);
    return true;
}
(Assignee)

Comment 5

5 years ago
Did you consider typeof Proxy.createFunction, or even the unholly window.all? But it might be a good idea to have a inline path for objects with the function or object class.

Comment 6

5 years ago
It works for Proxy.createFunction, because that's an function. I've checked in the browser with firebug what window.all should return, but it's returning undefined?
(Assignee)

Comment 7

5 years ago
I meant:
typeof Proxy.createFunction(function () {}, function () {})

In short, you need to call the hook in some cases, because the hook can return different for some objects.

Eg. window.all is actually an object, but we try to hide it by returning undefined (amongst doing other stuff).

Comment 8

5 years ago
Created attachment 587343 [details] [diff] [review]
just LIR::typeofObject with improvement

You're right. I've included an version of that function that also supports the proxy case. I don't know if it supports the window.all case.

I've implemented the algorithm like in jsobj.cpp: 6232:
return obj->isCallable() ? JSTYPE_FUNCTION : JSTYPE_OBJECT;
Bump -- Tom, what's the status on this op? I want us to stop aborting compilations in SunSpider. :-)

Comment 10

5 years ago
I left this patch for evilpie to complete. His version is complete, I've only made a suggestion for changing a part to MASM instead of VMCall. Because I'm going on holiday starting tomorrow, I cannot jump in and make a new patch... sorry.
Created attachment 595749 [details] [diff] [review]
Rebased

I'd like to get rid of the most common bailouts, so I reviewed and updated this for you. The most important changes are:

- For objects, we call a stub, since objects may have a custom typeof hook. At some point we may want to handle plain JS objects and functions without calling a stub, but getting rid of the aborts is more important right now.

- For other types we just emit type tests. The problem with using the type tag as index in some array is that it doesn't work for doubles. In the future we can get rid of type checks by looking at the possible types in the input typeset.

Tom, since you are the author of the patch, let me know if you agree with these changes.
Attachment #582824 - Attachment is obsolete: true
Attachment #582824 - Flags: review?(dvander)
Attachment #595749 - Flags: review?(evilpies)
(Assignee)

Comment 12

5 years ago
Comment on attachment 595749 [details] [diff] [review]
Rebased

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

This looks very clean to me :O. Yeah we need to see if we actually going to need something like TypeOfO. Besides that, a special optimization for typeof x == 'number' and a like (similar to what I did for JM) should definitely help, we should fill a followup for that.  I don't know if what you are doing with the oracle is correct (because i have problems understanding the interaction most of the time). But besides that everything is looking fine to me.

::: js/src/ion/MIR.h
@@ +1580,5 @@
> +        if (inputType_ <= MIRType_String)
> +            return AliasSet::None();
> +
> +        // For objects, typeof may invoke an effectful typeof hook.
> +        return AliasSet::Store(AliasSet::Any);

I hope that the typeof hooks don't do something like that actually :/
(Assignee)

Updated

5 years ago
Attachment #595749 - Flags: review?(evilpies) → review+
Thanks for the quick review, pushed with you as author:

http://hg.mozilla.org/projects/ionmonkey/rev/40112ee40593

Filed bug 725966 for the common "typeof x == y" case.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.