Bug 122306 - Add callOperation to Baseline JIT
Summary: Add callOperation to Baseline JIT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks: 122287
  Show dependency treegraph
 
Reported: 2013-10-03 17:15 PDT by Michael Saboff
Modified: 2013-10-04 11:19 PDT (History)
1 user (show)

See Also:


Attachments
Patch (12.30 KB, patch)
2013-10-03 17:23 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Patch with Updates from reviews (11.79 KB, patch)
2013-10-03 21:57 PDT, Michael Saboff
ggaren: review-
Details | Formatted Diff | Diff
Updated patch (14.22 KB, patch)
2013-10-04 10:27 PDT, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2013-10-03 17:15:46 PDT
Add callOperation framework to baseline JIT similar to what exists in DFG.
Comment 1 Michael Saboff 2013-10-03 17:23:19 PDT
Created attachment 213313 [details]
Patch

Converted cti_op_new_regexp() use to callOperation(operationNewRegexp()) as an example use.
Comment 2 Mark Lam 2013-10-03 17:28:49 PDT
Comment on attachment 213313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213313&action=review

> Source/JavaScriptCore/jit/JITOperations.h:176
> +void JIT_OPERATION operationStackCheck(ExecState*, void*) WTF_INTERNAL;

This line is not pertinent to this patch, is it?
Comment 3 Michael Saboff 2013-10-03 17:43:45 PDT
(In reply to comment #2)
> (From update of attachment 213313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213313&action=review
> 
> > Source/JavaScriptCore/jit/JITOperations.h:176
> > +void JIT_OPERATION operationStackCheck(ExecState*, void*) WTF_INTERNAL;
> 
> This line is not pertinent to this patch, is it?

I'll remove that.  It was part of something else I was trying.
Comment 4 Geoffrey Garen 2013-10-03 19:22:36 PDT
Comment on attachment 213313 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213313&action=review

> Source/JavaScriptCore/jit/JITInlines.h:220
> +ALWAYS_INLINE MacroAssembler::Call JIT::appendCallWithExceptionCheckSetResult(const FunctionPtr& function, int dst)
> +{
> +    MacroAssembler::Call call = appendCallWithExceptionCheck(function);
> +    emitStoreCell(dst, returnValueRegister);
> +    return call;
> +}

This doesn't look right. The return value from a stub is not guaranteed to be a cell. This will break when we start migrating more functions.
Comment 5 Michael Saboff 2013-10-03 21:09:24 PDT
(In reply to comment #4)
> (From update of attachment 213313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213313&action=review
> 
> > Source/JavaScriptCore/jit/JITInlines.h:220
> > +ALWAYS_INLINE MacroAssembler::Call JIT::appendCallWithExceptionCheckSetResult(const FunctionPtr& function, int dst)
> > +{
> > +    MacroAssembler::Call call = appendCallWithExceptionCheck(function);
> > +    emitStoreCell(dst, returnValueRegister);
> > +    return call;
> > +}
> 
> This doesn't look right. The return value from a stub is not guaranteed to be a cell. This will break when we start migrating more functions.

I'll make this version's name Cell specific.
Comment 6 Michael Saboff 2013-10-03 21:57:55 PDT
Created attachment 213334 [details]
Patch with Updates from reviews
Comment 7 Geoffrey Garen 2013-10-04 00:02:41 PDT
Comment on attachment 213334 [details]
Patch with Updates from reviews

View in context: https://bugs.webkit.org/attachment.cgi?id=213334&action=review

> Source/JavaScriptCore/jit/JITInlines.h:232
> +ALWAYS_INLINE MacroAssembler::Call JIT::callOperation(J_JITOperation_E operation, int dst)
> +{
> +    setupArgumentsExecState();
> +    return appendCallWithExceptionCheckSetCellResult(operation, dst);
> +}
> +
> +ALWAYS_INLINE MacroAssembler::Call JIT::callOperation(J_JITOperation_EP operation, int dst, void* pointer)
> +{
> +    setupArgumentsWithExecState(TrustedImmPtr(pointer));
> +    return appendCallWithExceptionCheckSetCellResult(operation, dst);
> +}

I don't think you understood my meaning. The function types here are J_JITOperation_E and J_JITOperation_EP:

typedef EncodedJSValue JIT_OPERATION (*J_JITOperation_E)(ExecState*);
typedef EncodedJSValue JIT_OPERATION (*J_JITOperation_EP)(ExecState*, void*);

It's inappropriate for a utility function that calls a function returning EncodedJSValue to assume that the return value is a cell. That's the assembly equivalent of an invalid cast.

You should either change the return type of operationNewRegexp to be JSCell*, or you should change these call sites to store a full EncodedJSValue.
Comment 8 Michael Saboff 2013-10-04 10:27:41 PDT
Created attachment 213372 [details]
Updated patch
Comment 9 Geoffrey Garen 2013-10-04 10:59:45 PDT
Comment on attachment 213372 [details]
Updated patch

r=me
Comment 10 Michael Saboff 2013-10-04 11:19:24 PDT
Committed r156896: <http://trac.webkit.org/changeset/156896>