| Summary: | B3-O2 incorrectly optimizes this subtest | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||
| Component: | JavaScriptCore | Assignee: | Robin Morisset <rmorisset> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | benjamin, commit-queue, fpizlo, ggaren, gskachkov, guijemont, keith_miller, mark.lam, mcatanzaro, msaboff, rmorisset, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
(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". 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).
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. Created attachment 362428 [details]
Patch
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? Created attachment 362436 [details]
Patch
Comment on attachment 362436 [details] Patch Clearing flags on attachment: 362436 Committed r241783: <https://trac.webkit.org/changeset/241783> All reviewed patches have been landed. Closing bug. 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 (‘!’)?
(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 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;” |
``` 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.