Bug 150762 - B3 should have a Select opcode
Summary: B3 should have a Select opcode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 150761 151616
Blocks: 151504 150279 151428
  Show dependency treegraph
 
Reported: 2015-10-31 16:50 PDT by Filip Pizlo
Modified: 2015-11-26 04:15 PST (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2015-10-31 16:50:24 PDT
Because conditional moves are pretty neat.
Comment 1 Filip Pizlo 2015-11-20 11:50:26 PST
Created attachment 265976 [details]
work in progress
Comment 2 Filip Pizlo 2015-11-20 13:28:21 PST
Created attachment 265988 [details]
the patch
Comment 3 WebKit Commit Bot 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.
Comment 4 Filip Pizlo 2015-11-20 13:58:19 PST
Created attachment 265995 [details]
the patch
Comment 5 WebKit Commit Bot 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.
Comment 6 Benjamin Poulain 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
Comment 7 Filip Pizlo 2015-11-20 14:31:57 PST
Landed in http://trac.webkit.org/changeset/192699
Comment 8 Filip Pizlo 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!