Bug 152955 - B3 should reduce Trunc(BitOr(value, constant)) where !(constant & 0xffffffff) to Trunc(value)
Summary: B3 should reduce Trunc(BitOr(value, constant)) where !(constant & 0xffffffff)...
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:
Blocks: 150507
  Show dependency treegraph
 
Reported: 2016-01-09 20:59 PST by Filip Pizlo
Modified: 2016-01-10 19:26 PST (History)
5 users (show)

See Also:


Attachments
the patch (6.39 KB, patch)
2016-01-09 21:01 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (6.49 KB, patch)
2016-01-10 17:16 PST, Filip Pizlo
sbarati: 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 2016-01-09 20:59:45 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2016-01-09 21:01:26 PST
Created attachment 268639 [details]
the patch
Comment 2 WebKit Commit Bot 2016-01-09 21:02:46 PST
Attachment 268639 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:9014:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:9032:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:9051:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:9070:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Filip Pizlo 2016-01-09 21:43:25 PST
This is a 2x speed-up on AsmBench/FloatMM.  It's neutral elsewhere.
Comment 4 Saam Barati 2016-01-10 15:02:49 PST
Comment on attachment 268639 [details]
the patch

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

I have a few questions, maybe I'm misunderstanding how some things.

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:809
> +            case Add:

Wouldn't this produce zero?

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:810
> +            case Sub:

is this signed or unsigned subtraction? If it's signed, why is this?
If it's unsigned, wouldn't this just be zero?

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:813
> +            case BitAnd:
> +            case BitOr:
> +            case BitXor:

Wouldn't these produce zero?
Comment 5 Saam Barati 2016-01-10 15:13:12 PST
(In reply to comment #4)
> Comment on attachment 268639 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268639&action=review
> 
> I have a few questions, maybe I'm misunderstanding how some things.
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:809
> > +            case Add:
> 
> Wouldn't this produce zero?
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:810
> > +            case Sub:
> 
> is this signed or unsigned subtraction? If it's signed, why is this?
> If it's unsigned, wouldn't this just be zero?
Never mind about signedness, my question is just if this would
always be zero?

> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:813
> > +            case BitAnd:
> > +            case BitOr:
> > +            case BitXor:
> 
> Wouldn't these produce zero?
Comment 6 Filip Pizlo 2016-01-10 16:51:51 PST
(In reply to comment #4)
> Comment on attachment 268639 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268639&action=review
> 
> I have a few questions, maybe I'm misunderstanding how some things.
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:809
> > +            case Add:
> 
> Wouldn't this produce zero?

The expression in question is:

    static_cast<int32_t>(x + const)

Where "static_cast<int32>()" is Trunc and "x + const" is Add.

In this case, if const1 does not have any of its low 32 bits set, we are guaranteed that the expression is equivalent to:

    static_cast<int32_t>(x)

That's guaranteed because if const has zero in all of its low 32 bits, then "x + const" does not change anything about the low 32 bits of x.

That also means that this does not produce zero.  It does not produce zero because it simply produces static_cast<int32>(x).

> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:810
> > +            case Sub:
> 
> is this signed or unsigned subtraction? If it's signed, why is this?
> If it's unsigned, wouldn't this just be zero?

There is no difference between signed subtraction and unsigned subtraction.

> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:813
> > +            case BitAnd:
> > +            case BitOr:
> > +            case BitXor:
> 
> Wouldn't these produce zero?

No.  If you do:

    static_cast<int32_t>(x | 0xabcdef1200000000)

then it's equivalent to:

    static_cast<int32_t>(x)

Because the "|" operation could only have affected the top bits.  That's why there is a check that the constant has zero in all of its low bits.  If it has zero in all of its low bits, then we can short-circuit the Trunc around the "|".

The same is true for "^" and "&".
Comment 7 Filip Pizlo 2016-01-10 16:52:08 PST
(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 268639 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=268639&action=review
> > 
> > I have a few questions, maybe I'm misunderstanding how some things.
> > 
> > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:809
> > > +            case Add:
> > 
> > Wouldn't this produce zero?
> > 
> > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:810
> > > +            case Sub:
> > 
> > is this signed or unsigned subtraction? If it's signed, why is this?
> > If it's unsigned, wouldn't this just be zero?
> Never mind about signedness, my question is just if this would
> always be zero?

Answered above.
Comment 8 Saam Barati 2016-01-10 17:08:35 PST
Comment on attachment 268639 [details]
the patch

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

>>>>> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:809
>>>>> +            case Add:
>>>> 
>>>> Wouldn't this produce zero?
>>> 
>>> Never mind about signedness, my question is just if this would
>>> always be zero?
>> 
>> The expression in question is:
>> 
>>     static_cast<int32_t>(x + const)
>> 
>> Where "static_cast<int32>()" is Trunc and "x + const" is Add.
>> 
>> In this case, if const1 does not have any of its low 32 bits set, we are guaranteed that the expression is equivalent to:
>> 
>>     static_cast<int32_t>(x)
>> 
>> That's guaranteed because if const has zero in all of its low 32 bits, then "x + const" does not change anything about the low 32 bits of x.
>> 
>> That also means that this does not produce zero.  It does not produce zero because it simply produces static_cast<int32>(x).
> 
> Answered above.

I see. I had a total brain fart when I was reasoning about this. Thanks for the clarification. It makes sense.

>>>> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:813
>>>> +            case BitXor:
>>> 
>>> Wouldn't these produce zero?
>> 
>> 
> 
> No.  If you do:
> 
>     static_cast<int32_t>(x | 0xabcdef1200000000)
> 
> then it's equivalent to:
> 
>     static_cast<int32_t>(x)
> 
> Because the "|" operation could only have affected the top bits.  That's why there is a check that the constant has zero in all of its low bits.  If it has zero in all of its low bits, then we can short-circuit the Trunc around the "|".
> 
> The same is true for "^" and "&".

I agree with BitOr and BitXor.
For BitAnd, this seems wrong. If we're ANDing something with something else with all lower 32-bits are zero, we should transform this into zero.
Comment 9 Filip Pizlo 2016-01-10 17:16:21 PST
(In reply to comment #8)
> Comment on attachment 268639 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268639&action=review
> 
> >>>>> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:809
> >>>>> +            case Add:
> >>>> 
> >>>> Wouldn't this produce zero?
> >>> 
> >>> Never mind about signedness, my question is just if this would
> >>> always be zero?
> >> 
> >> The expression in question is:
> >> 
> >>     static_cast<int32_t>(x + const)
> >> 
> >> Where "static_cast<int32>()" is Trunc and "x + const" is Add.
> >> 
> >> In this case, if const1 does not have any of its low 32 bits set, we are guaranteed that the expression is equivalent to:
> >> 
> >>     static_cast<int32_t>(x)
> >> 
> >> That's guaranteed because if const has zero in all of its low 32 bits, then "x + const" does not change anything about the low 32 bits of x.
> >> 
> >> That also means that this does not produce zero.  It does not produce zero because it simply produces static_cast<int32>(x).
> > 
> > Answered above.
> 
> I see. I had a total brain fart when I was reasoning about this. Thanks for
> the clarification. It makes sense.
> 
> >>>> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:813
> >>>> +            case BitXor:
> >>> 
> >>> Wouldn't these produce zero?
> >> 
> >> 
> > 
> > No.  If you do:
> > 
> >     static_cast<int32_t>(x | 0xabcdef1200000000)
> > 
> > then it's equivalent to:
> > 
> >     static_cast<int32_t>(x)
> > 
> > Because the "|" operation could only have affected the top bits.  That's why there is a check that the constant has zero in all of its low bits.  If it has zero in all of its low bits, then we can short-circuit the Trunc around the "|".
> > 
> > The same is true for "^" and "&".
> 
> I agree with BitOr and BitXor.
> For BitAnd, this seems wrong. If we're ANDing something with something else
> with all lower 32-bits are zero, we should transform this into zero.

Good point.  I'll remove the BitAnd from the switch statement.
Comment 10 Filip Pizlo 2016-01-10 17:16:56 PST
Created attachment 268667 [details]
the patch
Comment 11 WebKit Commit Bot 2016-01-10 17:18:21 PST
Attachment 268667 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:9015:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:9033:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:9052:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:9071:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Saam Barati 2016-01-10 17:49:49 PST
Comment on attachment 268667 [details]
the patch

r=me
Comment 13 Filip Pizlo 2016-01-10 19:26:42 PST
Landed in http://trac.webkit.org/changeset/194836