691373 - IonMonkey: Implement JSOP_TYPEOF

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

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

Implement the opcode  JSOP_TYPEOF in ionmonkey.

Comment 1

[Tom S [:evilpie]]

Attachment: 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.

Comment 2

[Tom S [:evilpie]]

Attachment: 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.

Comment 3

[Tom S [:evilpie]]

Attachment: 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

Comment 4

[kosver]

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;
}

Comment 5

[Tom S [:evilpie]]

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

[kosver]

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?

Comment 7

[Tom S [:evilpie]]

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

[kosver]

Attachment: 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;

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

Bump -- Tom, what's the status on this op? I want us to stop aborting compilations in SunSpider. :-)

Comment 10

[kosver]

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.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 12

[Tom S [:evilpie]]

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 :/

Comment 13

[Jan de Mooij [:jandem]]

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.