Bug 122627

Summary: DFG: Add JIT support for LogicalNot(String/StringIdent)
Product: WebKit Reporter: Nadav Rotem <nrotem>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Nadav Rotem 2013-10-10 17:42:01 PDT
DFG: Add JIT support for  LogicalNot(String/StringIdent)
Comment 1 Nadav Rotem 2013-10-10 17:42:22 PDT
Created attachment 213950 [details]
Patch
Comment 2 Filip Pizlo 2013-10-10 18:46:36 PDT
Comment on attachment 213950 [details]
Patch

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

Almost good, just needs a few fixes.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:573
>                  break;
> +            case StringUse:
>              case ObjectOrOtherUse:

I think that the StringUse case should be in the case block above, since the only reason why it would exit is for the type check.  AbstractInterpreter already will setCanExit(true) if it determines that the type check implied by the use kind is necessary.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:296
> +            else if (node->child1()->shouldSpeculateString() || node->child1()->shouldSpeculateStringIdent())
> +                fixEdge<StringUse>(node->child1());

shouldSpeculateString() should subsume shouldSpeculateStringIdent(), so you don't need the second part.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4170
> +void SpeculativeJIT::compileStringIdentZeroLength(Node* node)

See below about method name.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1663
> +    case StringUse:
> +        return compileStringIdentZeroLength(node);
> +

I wouldn't call this method compileStringIdentZeroLength.  It doesn't have anything to do with StringIdent - you're not requiring the operand to be an identifier, you're only requiring it to be a string.  So, a name like compileStringZeroLength would be fine.
Comment 3 Nadav Rotem 2013-10-11 10:43:27 PDT
Created attachment 214001 [details]
Patch
Comment 4 Nadav Rotem 2013-10-11 10:44:11 PDT
Thanks Filip. I fixed the patch to address your comments. :)
Comment 5 Filip Pizlo 2013-10-11 10:52:38 PDT
Comment on attachment 214001 [details]
Patch

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

Can you also add a test?  Best to add one in LayoutTests/js, and use the dfgShouldBe function to drive the test. There should be examples in that directory that do it. This will automatically run the test long enough to tier-up into the DFG.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4184
> +    m_jit.load32(MacroAssembler::Address(strGPR, JSString::offsetOfLength()), lenGPR);

Why not use compare32() that takes an Address and a TrustedImm32?  It will give you the same functionality with just one instruction on x86 and I think just two on arm.
Comment 6 Nadav Rotem 2013-10-11 11:09:11 PDT
compare32 does not take an address, but I will fold the immediate into the comparison and add a test case.
Comment 7 Filip Pizlo 2013-10-11 11:12:07 PDT
(In reply to comment #6)
> compare32 does not take an address, but I will fold the immediate into the comparison and add a test case.

Oh, right!  But there is this thing:

    void test32(ResultCondition cond, Address address, TrustedImm32 mask, RegisterID dest)

You'd use TrustedImm32(-1) as the mask.  This will Do The Right Thing on both x86 and arm.  On x86 you'll get a cmpl $0, (memory). On arm you'll get a ldr and tst.
Comment 8 Nadav Rotem 2013-10-11 13:25:01 PDT
Created attachment 214019 [details]
Patch
Comment 9 Nadav Rotem 2013-10-11 13:26:08 PDT
Switched to test32 and added a test case. I verified with a debugger that compileStringZeroLength is called.
Comment 10 Filip Pizlo 2013-10-11 13:33:15 PDT
Comment on attachment 214019 [details]
Patch

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

> LayoutTests/js/script-tests/test_not_string.js:15
> +
> +function foo(text) {
> +  return !!text
> +}
> +
> +var sum = 0;
> +var str = ""
> +for (var i=0; i < 1000000; i++) {
> +  sum += foo(str)
> +
> +  if (sum < 10)
> +    str += "a"
> +}
> +
> +shouldBe("sum", "999999");

- Tests for the DFG are usually called "dfg-<blah>.js"

- We usually use "-" as a word separator in test names.

- This test appears to rely on 1000000 being enough runs to trigger the DFG.  That's actually probably good enough, but you could make the test more robust by using the dfgShouldBe function:

dfgShouldBe(foo, "foo(\"\")", "true");
dfgShouldBe(foo, "foo(\"blah\")", "false");

You can alternatively do this sort of manually, like:

noInline(foo);
while (!dfgCompiled({f:foo}))
    foo( .... some arguments to warm it up ...);

shouldBe("foo(...   // your actual tests here
Comment 11 Nadav Rotem 2013-10-11 13:51:53 PDT
Created attachment 214021 [details]
Patch
Comment 12 Filip Pizlo 2013-10-11 14:05:20 PDT
Comment on attachment 214021 [details]
Patch

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

> LayoutTests/js/script-tests/dfg-not-string.js:15
> +
> +function foo(text) {
> +  return !!text
> +}
> +
> +var sum = 0;
> +var str = ""
> +for (var i=0; i < 1000; i++) {
> +  sum += foo(str)
> +
> +  if (sum < 10)
> +    str += "a"
> +}
> +
> +dfgShouldBe(foo, "sum", "999");

I think this will deadlock in some cases.  dfgShouldBe()'s second argument should be an expression that results in a call to the function you pass in the first argument.
Comment 13 Nadav Rotem 2013-10-11 14:30:50 PDT
Created attachment 214023 [details]
Patch
Comment 14 Filip Pizlo 2013-10-11 14:34:03 PDT
Comment on attachment 214023 [details]
Patch

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

> LayoutTests/js/script-tests/dfg-not-string.js:17
> +dfgShouldBe(test, "test()", "9");

This isn't guaranteed to run long enough to get foo() to tier up.  It'll wait for test() to tier up.  Normally, our heuristics will compile foo() before test() because foo() is smaller and is called more frequently, but we shouldn't be relying on heuristics in the tests.  You can fix this by saying:

dfgShouldBe(foo, "test()", "9");
Comment 15 Nadav Rotem 2013-10-11 17:03:02 PDT
got it. I assumed that if Test is jitted then Foo is also jitter because it is called more frequently.  I will change the test case and resubmit. Thanks for the review :)
Comment 16 Nadav Rotem 2013-10-11 17:06:45 PDT
Created attachment 214037 [details]
Patch
Comment 17 WebKit Commit Bot 2013-10-11 19:20:14 PDT
Comment on attachment 214037 [details]
Patch

Clearing flags on attachment: 214037

Committed r157329: <http://trac.webkit.org/changeset/157329>
Comment 18 WebKit Commit Bot 2013-10-11 19:20:16 PDT
All reviewed patches have been landed.  Closing bug.