Bug 150762

Summary:

B3 should have a Select opcode

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, benjamin, commit-queue, ggaren, keith_miller, mark.lam, mhahnenb, msaboff, nrotem, oliver, saam, sam

Priority:

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

150761, 151616

Bug Blocks:

151504, 150279, 151428

Attachments:

Description	Flags
work in progress	none
the patch	none
the patch	benjamin: review+

Filip Pizlo

Reported 2015-10-31 16:50:24 PDT

Because conditional moves are pretty neat.

Attachments
work in progress (22.36 KB, patch) 2015-11-20 11:50 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (39.63 KB, patch) 2015-11-20 13:28 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (44.55 KB, patch) 2015-11-20 13:58 PST, Filip Pizlo	benjamin: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2015-11-20 11:50:26 PST

Created attachment 265976 [details] work in progress

Filip Pizlo

Comment 2 2015-11-20 13:28:21 PST

Created attachment 265988 [details] the patch

WebKit Commit Bot

Comment 3 2015-11-20 13:30:37 PST

Attachment 265988 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:847: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/b3/B3ValueKey.h:65: The parameter name "a" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4751: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4752: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4753: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4754: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4770: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4771: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4772: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4773: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4792: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4793: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4794: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4795: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4796: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/B3Value.h:238: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/b3/B3Value.h:248: Missing space before { [whitespace/braces] [5] Total errors found: 17 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 4 2015-11-20 13:58:19 PST

Created attachment 265995 [details] the patch

WebKit Commit Bot

Comment 5 2015-11-20 14:00:47 PST

Attachment 265995 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3Value.h:238: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/b3/B3Value.h:248: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/b3/B3ValueKey.h:65: The parameter name "a" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4751: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4752: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4753: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4754: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4770: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4771: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4772: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4773: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4792: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4793: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4794: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4795: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4796: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4902: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4903: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4904: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4905: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:847: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 21 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Benjamin Poulain

Comment 6 2015-11-20 14:10:21 PST

Comment on attachment 265995 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=265995&action=review > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:555 > + && m_value->child(0)->child(1)->isInt32(1) Should be ->asInt() & 1 > Source/JavaScriptCore/dfg/DFGCommon.h:41 > -#define FTL_USES_B3 0 > +#define FTL_USES_B3 1 Oops

Filip Pizlo

Comment 7 2015-11-20 14:31:57 PST

Landed in http://trac.webkit.org/changeset/192699

Filip Pizlo

Comment 8 2015-11-20 14:35:28 PST

(In reply to comment #6) > Comment on attachment 265995 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265995&action=review > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:555 > > + && m_value->child(0)->child(1)->isInt32(1) > > Should be ->asInt() & 1 I think that would be wrong. If you take a boolean value - i.e. 0 or 1 - and xor it with a value that is not 1 but has the 1 bit set, like say 41, then you're not actually doing a logical not. > > > Source/JavaScriptCore/dfg/DFGCommon.h:41 > > -#define FTL_USES_B3 0 > > +#define FTL_USES_B3 1 > > Oops Fixed!

Note You need to log in before you can comment on or make changes to this bug.