Bug 194625 - B3-O2 incorrectly optimizes this subtest
Summary: B3-O2 incorrectly optimizes this subtest
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-13 16:51 PST by Saam Barati
Modified: 2019-02-24 13:23 PST (History)
15 users (show)

See Also:


Attachments
Patch (2.36 KB, patch)
2019-02-19 14:21 PST, Robin Morisset
sbarati: review+
sbarati: commit-queue-
Details | Formatted Diff | Diff
Patch (4.60 KB, patch)
2019-02-19 14:49 PST, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2019-02-13 16:51:11 PST
```
void testBitNotOnBooleanAndBranch32(int64_t a, int64_t b)
{
    Procedure proc;
    BasicBlock* root = proc.addBlock();
    BasicBlock* thenCase = proc.addBlock();
    BasicBlock* elseCase = proc.addBlock();

    Value* arg1 = root->appendNew<Value>(proc, Trunc, Origin(),
        root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0));
    Value* arg2 = root->appendNew<Value>(proc, Trunc, Origin(),
        root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR1));
    Value* argsAreEqual = root->appendNew<Value>(proc, Equal, Origin(), arg1, arg2);
    Value* argsAreNotEqual = root->appendNew<Value>(proc, BitXor, Origin(),
        root->appendNew<Const32Value>(proc, Origin(), -1),
        argsAreEqual);

    root->appendNewControlValue(
        proc, Branch, Origin(),
        argsAreNotEqual,
        FrequentedBlock(thenCase), FrequentedBlock(elseCase));

    thenCase->appendNewControlValue(
        proc, Return, Origin(),
        thenCase->appendNew<Const32Value>(proc, Origin(), 42));

    elseCase->appendNewControlValue(
        proc, Return, Origin(),
        elseCase->appendNew<Const32Value>(proc, Origin(), -42));

    int32_t expectedValue = (a != b) ? 42 : -42;
    int32_t result = compileAndRun<int32_t>(proc, a, b);
    CHECK(result == expectedValue);
}
```

By my understanding of B3, I think this should always be "argsAreNotEqual" as truthy. And we should always return 42.

Interstingly, B3-02 does what I think the author of this test expected to happen, and to Xor with "1", not "-1". So I think this actually implicates a bug in B3 itself. Perhaps in isel.
Comment 1 Saam Barati 2019-02-13 16:53:06 PST
(In reply to Saam Barati from comment #0)
> ```
> void testBitNotOnBooleanAndBranch32(int64_t a, int64_t b)
> {
>     Procedure proc;
>     BasicBlock* root = proc.addBlock();
>     BasicBlock* thenCase = proc.addBlock();
>     BasicBlock* elseCase = proc.addBlock();
> 
>     Value* arg1 = root->appendNew<Value>(proc, Trunc, Origin(),
>         root->appendNew<ArgumentRegValue>(proc, Origin(),
> GPRInfo::argumentGPR0));
>     Value* arg2 = root->appendNew<Value>(proc, Trunc, Origin(),
>         root->appendNew<ArgumentRegValue>(proc, Origin(),
> GPRInfo::argumentGPR1));
>     Value* argsAreEqual = root->appendNew<Value>(proc, Equal, Origin(),
> arg1, arg2);
>     Value* argsAreNotEqual = root->appendNew<Value>(proc, BitXor, Origin(),
>         root->appendNew<Const32Value>(proc, Origin(), -1),
>         argsAreEqual);
> 
>     root->appendNewControlValue(
>         proc, Branch, Origin(),
>         argsAreNotEqual,
>         FrequentedBlock(thenCase), FrequentedBlock(elseCase));
> 
>     thenCase->appendNewControlValue(
>         proc, Return, Origin(),
>         thenCase->appendNew<Const32Value>(proc, Origin(), 42));
> 
>     elseCase->appendNewControlValue(
>         proc, Return, Origin(),
>         elseCase->appendNew<Const32Value>(proc, Origin(), -42));
> 
>     int32_t expectedValue = (a != b) ? 42 : -42;
>     int32_t result = compileAndRun<int32_t>(proc, a, b);
>     CHECK(result == expectedValue);
> }
> ```
> 
> By my understanding of B3, I think this should always be "argsAreNotEqual"
> as truthy. And we should always return 42.
> 
> Interstingly, B3-02 does what I think the author of this test expected to
> happen, and to Xor with "1", not "-1". So I think this actually implicates a
> bug in B3 itself. Perhaps in isel.

This last sentence was written badly: B3 doesn't Xor with 1. It has the same effect though. It ends up fusing to just be a BranchCompare. I meant that I think the author intended for this to be "1", not "-1".
Comment 2 Robin Morisset 2019-02-13 16:55:17 PST
I agree and understand the code like you: Branch considers everything different from 0 as being true.
It is an easy mistake, as we are not always super explicit between the boolean not ('!' in C, BitXor with 1 in B3) and the binary negation ('~' in C, BitXor with -1 in B3).
Comment 3 Saam Barati 2019-02-17 12:44:17 PST
I'm fixing this subtest in:
https://bugs.webkit.org/show_bug.cgi?id=194637

But we should use this bug to figure out why O2 incorrectly optimizes this code.
Comment 4 Robin Morisset 2019-02-19 14:21:06 PST
Created attachment 362428 [details]
Patch
Comment 5 Saam Barati 2019-02-19 14:24:32 PST
Comment on attachment 362428 [details]
Patch

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

r=me

> Source/JavaScriptCore/b3/testb3.cpp:3537
> +        root->appendNew<Const32Value>(proc, Origin(), 1),

Can you add a test that would have done the wrong in thing in B3-O2 before?
Comment 6 Robin Morisset 2019-02-19 14:49:54 PST
Created attachment 362436 [details]
Patch
Comment 7 WebKit Commit Bot 2019-02-19 15:27:21 PST
Comment on attachment 362436 [details]
Patch

Clearing flags on attachment: 362436

Committed r241783: <https://trac.webkit.org/changeset/241783>
Comment 8 WebKit Commit Bot 2019-02-19 15:27:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-02-19 15:30:57 PST
<rdar://problem/48217706>
Comment 10 Michael Catanzaro 2019-02-23 12:46:18 PST
Robin, what were you trying to do here?

[2491/4022] Building CXX object Source...akeFiles/testb3.dir/__/b3/testb3.cpp.o
../../Source/JavaScriptCore/b3/testb3.cpp: In function ‘void {anonymous}::testBitNotOnBooleanAndBranch32(int64_t, int64_t)’:
../../Source/JavaScriptCore/b3/testb3.cpp:3752:33: warning: ‘~’ on an expression of type bool [-Wbool-operation]
     int32_t expectedValue = ~(a == b) ? 42 : -42; // always 42
                              ~~~^~~~~
../../Source/JavaScriptCore/b3/testb3.cpp:3752:33: note: did you mean to use logical not (‘!’)?
Comment 11 Saam Barati 2019-02-24 13:23:10 PST
(In reply to Michael Catanzaro from comment #10)
> Robin, what were you trying to do here?
> 
> [2491/4022] Building CXX object
> Source...akeFiles/testb3.dir/__/b3/testb3.cpp.o
> ../../Source/JavaScriptCore/b3/testb3.cpp: In function ‘void
> {anonymous}::testBitNotOnBooleanAndBranch32(int64_t, int64_t)’:
> ../../Source/JavaScriptCore/b3/testb3.cpp:3752:33: warning: ‘~’ on an
> expression of type bool [-Wbool-operation]
>      int32_t expectedValue = ~(a == b) ? 42 : -42; // always 42
>                               ~~~^~~~~
> ../../Source/JavaScriptCore/b3/testb3.cpp:3752:33: note: did you mean to use
> logical not (‘!’)?

We expect this to always be 42. It’s fine to remove the computation that the compiler proves is always 42.
Comment 12 Saam Barati 2019-02-24 13:23:58 PST
Comment on attachment 362436 [details]
Patch

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

> Source/JavaScriptCore/b3/testb3.cpp:3582
> +    int32_t expectedValue = ~(a == b) ? 42 : -42; // always 42

Specifically, just change this line to be: “... = 42;”