Last Comment Bug 691373 - IonMonkey: Implement JSOP_TYPEOF
: IonMonkey: Implement JSOP_TYPEOF
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: AWAY Tom Schuster [:evilpie]
:
:
Mentors:
Depends on: 680315
Blocks: 684381
  Show dependency treegraph
 
Reported: 2011-10-03 09:43 PDT by AWAY Tom Schuster [:evilpie]
Modified: 2012-02-10 05:17 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP v0 (5.46 KB, patch)
2011-10-04 12:47 PDT, AWAY Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
WIP v1 (14.86 KB, patch)
2011-12-09 07:58 PST, AWAY Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
v1 (14.86 KB, patch)
2011-12-19 07:45 PST, AWAY Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
just LIR::typeofObject with improvement (2.33 KB, patch)
2012-01-10 09:15 PST, kosver
no flags Details | Diff | Splinter Review
Rebased (23.75 KB, patch)
2012-02-09 08:05 PST, Jan de Mooij [:jandem]
evilpies: review+
Details | Diff | Splinter Review

Description AWAY Tom Schuster [:evilpie] 2011-10-03 09:43:24 PDT
+++ This bug was initially created as a clone of Bug #685838 +++

Implement the opcode  JSOP_TYPEOF in ionmonkey.
Comment 1 AWAY Tom Schuster [:evilpie] 2011-10-04 12:47:33 PDT
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.
Comment 2 AWAY Tom Schuster [:evilpie] 2011-12-09 07:58:06 PST
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.
Comment 3 AWAY Tom Schuster [:evilpie] 2011-12-19 07:45:07 PST
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
Comment 4 kosver 2012-01-10 05:33:34 PST
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 AWAY Tom Schuster [:evilpie] 2012-01-10 07:18:27 PST
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 2012-01-10 07:54:14 PST
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 AWAY Tom Schuster [:evilpie] 2012-01-10 07:59:59 PST
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 2012-01-10 09:15:05 PST
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;
Comment 9 Chris Leary [:cdleary] (not checking bugmail) 2012-02-03 04:03:18 PST
Bump -- Tom, what's the status on this op? I want us to stop aborting compilations in SunSpider. :-)
Comment 10 kosver 2012-02-03 09:44:54 PST
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] 2012-02-09 08:05:23 PST
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.
Comment 12 AWAY Tom Schuster [:evilpie] 2012-02-09 11:44:00 PST
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] 2012-02-10 05:17:07 PST
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.

Note You need to log in before you can comment on or make changes to this bug.