Bug 194625 – B3-O2 incorrectly optimizes this subtest

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

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

[Saam Barati]

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

[Robin Morisset]

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

[Saam Barati]

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.

[Robin Morisset]

Created attachment 362428 [details]
Patch

[Saam Barati]

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?

[Robin Morisset]

Created attachment 362436 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 362436 [details]
Patch

Clearing flags on attachment: 362436

Committed r241783: <https://trac.webkit.org/changeset/241783>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48217706>

[Michael Catanzaro]

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 (‘!’)?

[Saam Barati]

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

[Saam Barati]

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;”