Bug 152955 – B3 should reduce Trunc(BitOr(value, constant)) where !(constant & 0xffffffff) to Trunc(value)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 268639 [details]
the patch

[WebKit Commit Bot]

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.

[Filip Pizlo]

This is a 2x speed-up on AsmBench/FloatMM.  It's neutral elsewhere.

[Saam Barati]

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?

[Saam Barati]

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

[Filip Pizlo]

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

[Filip Pizlo]

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

[Saam Barati]

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.

[Filip Pizlo]

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

[Filip Pizlo]

Created attachment 268667 [details]
the patch

[WebKit Commit Bot]

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.

[Saam Barati]

Comment on attachment 268667 [details]
the patch

r=me

[Filip Pizlo]

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