Closed Bug 691373 Opened 13 years ago Closed 12 years ago

IonMonkey: Implement JSOP_TYPEOF

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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

Implement the opcode  JSOP_TYPEOF in ionmonkey.
Assignee: general → evilpies
No longer depends on: 691340
Attached patch WIP v0 (obsolete) — Splinter Review
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.
Attached patch WIP v1 (obsolete) — Splinter Review
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
Attached patch v1 (obsolete) — Splinter Review
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)
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;
}
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.
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?
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).
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. :-)
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.
Attached patch RebasedSplinter Review
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)
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 :/
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
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.