Bug 129051 - GetMyArgumentsByLength in FTL doesn't work with inline code
Summary: GetMyArgumentsByLength in FTL doesn't work with inline code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-19 10:36 PST by Matthew Mirman
Modified: 2014-02-21 13:47 PST (History)
3 users (show)

See Also:


Attachments
Fixed GetMyArgumentsLength in FTL to work with inline code (1.37 KB, patch)
2014-02-19 10:38 PST, Matthew Mirman
ggaren: review-
ggaren: commit-queue-
Details | Formatted Diff | Diff
Fixed GetMyArgumentsLength in FTL to work with inline code (1.99 KB, patch)
2014-02-19 10:51 PST, Matthew Mirman
fpizlo: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Fixed GetMyArgumentsLength in FTL to work with inline code (2.54 KB, patch)
2014-02-21 12:19 PST, Matthew Mirman
fpizlo: review-
fpizlo: commit-queue-
Details | Formatted Diff | Diff
Fixed GetMyArgumentsLength in FTL to work with inline code (1.72 KB, patch)
2014-02-21 12:57 PST, Matthew Mirman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Mirman 2014-02-19 10:36:45 PST
patch forthcoming.
Comment 1 Matthew Mirman 2014-02-19 10:38:50 PST
Created attachment 224651 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code
Comment 2 Geoffrey Garen 2014-02-19 10:44:48 PST
Comment on attachment 224651 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

Can you add a test case for this? (If it's already covered by an existing test, you should note that in your ChangeLog.)
Comment 3 Matthew Mirman 2014-02-19 10:51:54 PST
Created attachment 224654 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

Added a test case.
Comment 4 Geoffrey Garen 2014-02-19 11:56:18 PST
Comment on attachment 224654 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

dfgShouldBe is a slightly better way to write a test like this. It tests precisely whether the function made it into our optimizing compilers, which means the test can terminate sooner, and there's no risk that the test will terminate too soon. You should make that change in a follow-up patch.
Comment 5 WebKit Commit Bot 2014-02-19 11:58:58 PST
Comment on attachment 224654 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

Rejecting attachment 224654 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 224654, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
kitpy/common/checkout/changelog.py", line 347, in parse_entries_from_file
    yield ChangeLogEntry(''.join(entry_lines[:-1]), revision=most_probable_revision)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/checkout/changelog.py", line 112, in __init__
    self._parse_entry()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/checkout/changelog.py", line 206, in _parse_entry
    self._date_line = match.group()
AttributeError: 'NoneType' object has no attribute 'group'

Full output: http://webkit-queues.appspot.com/results/5252063646711808
Comment 6 Filip Pizlo 2014-02-20 13:53:46 PST
Comment on attachment 224654 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

Actually, this test is broken.  There's no way it even hits the FTL since it relies on global code.  Did you test the test?  It's very important that when you add a test, you check that it indeed reproduces the bug you're trying to fix.  I don't buy that this test gives us coverage over the old version of this code.
Comment 7 Matthew Mirman 2014-02-21 12:19:51 PST
Created attachment 224890 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code
Comment 8 Filip Pizlo 2014-02-21 12:30:31 PST
Comment on attachment 224890 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:300
> +        case GetMyArgumentsLengthSafe:

I don't think you want to combine these two.  The purpose of GetMyArgumenstLengthSafe is to have a slow path, rather than a speculation, for the arguments-created case.  See what the DFG does, notice the slow path call:

    case GetMyArgumentsLengthSafe: {
        GPRTemporary result(this);
        GPRReg resultGPR = result.gpr();
        
        JITCompiler::Jump created = m_jit.branchTest64(
            JITCompiler::NonZero,
            JITCompiler::addressFor(
                m_jit.graph().machineArgumentsRegisterFor(node->origin.semantic)));
        
        if (node->origin.semantic.inlineCallFrame) {
            m_jit.move(
                Imm64(JSValue::encode(jsNumber(node->origin.semantic.inlineCallFrame->arguments.size() - 1))),
                resultGPR);
        } else {
            m_jit.load32(JITCompiler::payloadFor(JSStack::ArgumentCount), resultGPR);
            m_jit.sub32(TrustedImm32(1), resultGPR);
            m_jit.or64(GPRInfo::tagTypeNumberRegister, resultGPR);
        }
        
        // FIXME: the slow path generator should perform a forward speculation that the
        // result is an integer. For now we postpone the speculation by having this return
        // a JSValue.
        
        addSlowPathGenerator(
            slowPathCall(
                created, this, operationGetArgumentsLength, resultGPR,
                m_jit.graph().machineArgumentsRegisterFor(node->origin.semantic).offset()));
        
        jsValueResult(resultGPR, node);
        break;
    }
Comment 9 Matthew Mirman 2014-02-21 12:57:18 PST
Created attachment 224894 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code
Comment 10 WebKit Commit Bot 2014-02-21 13:47:28 PST
Comment on attachment 224894 [details]
Fixed GetMyArgumentsLength in FTL to work with inline code

Clearing flags on attachment: 224894

Committed r164496: <http://trac.webkit.org/changeset/164496>
Comment 11 WebKit Commit Bot 2014-02-21 13:47:30 PST
All reviewed patches have been landed.  Closing bug.