DFG: Add JIT support for LogicalNot(String/StringIdent)

[Nadav Rotem]

DFG: Add JIT support for  LogicalNot(String/StringIdent)

[Nadav Rotem]

Created attachment 213950 [details]
Patch

[Filip Pizlo]

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.

[Nadav Rotem]

Created attachment 214001 [details]
Patch

[Nadav Rotem]

Thanks Filip. I fixed the patch to address your comments. :)

[Filip Pizlo]

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.

[Nadav Rotem]

compare32 does not take an address, but I will fold the immediate into the comparison and add a test case.

[Filip Pizlo]

(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.

[Nadav Rotem]

Created attachment 214019 [details]
Patch

[Nadav Rotem]

Switched to test32 and added a test case. I verified with a debugger that compileStringZeroLength is called.

[Filip Pizlo]

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

[Nadav Rotem]

Created attachment 214021 [details]
Patch

[Filip Pizlo]

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.

[Nadav Rotem]

Created attachment 214023 [details]
Patch

[Filip Pizlo]

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");

[Nadav Rotem]

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 :)

[Nadav Rotem]

Created attachment 214037 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 214037 [details]
Patch

Clearing flags on attachment: 214037

Committed r157329: <http://trac.webkit.org/changeset/157329>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.