Bug 167161 – [B3] B3 strength reduction could encounter Value without owner in PureCSE

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

[Yusuke Suzuki]

B3 strength reduction clears existing value->owner in specializeSelect.
Thus, PureCSE's stored value `match` may not have `owner`.
This is caused in webassembly.org's demo.

[Yusuke Suzuki]

Created attachment 299142 [details]
Patch

[WebKit Commit Bot]

Attachment 299142 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:11873:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:11874:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:11874:  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/testb3.cpp:11875:  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.

[Build Bot]

Comment on attachment 299142 [details]
Patch

Attachment 299142 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2910022

New failing tests:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-containers-styles.html

[Build Bot]

Created attachment 299146 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Filip Pizlo]

Comment on attachment 299142 [details]
Patch

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

Nice!  r=me.

> Source/JavaScriptCore/ChangeLog:20
> +        In this patch, we just ignore the value with owner = nullptr. It is OK because
> +
> +        1. Still this value itself is valid. It is converted to Phi and it is placed in a newly
> +        created basic block. And this basic block still dominates basic blocks that are previously
> +        dominated by the original basic block.
> +        2. Even if we ignore the chance to optimize it, we will encounter the same chance later
> +        since `m_changed = true` is specified.

I think that the burden of proof for your change is much lower: you can ignore the value with owner = nullptr because:

1. The value's memory is indeed still valid. The fact that the memory is valid permits us to load Value::owner and testing it is meaningful. In your new improved algorithm, it does not matter what the state of Values with null owners is so long as they are owned by Procedure (i.e. the value's memory is still valid). For example, it doesn't matter what the dominance situation of Values with null owners is.

2. PureCSE is allowed to be conservative. The compiler does not depend on PureCSE to always succeed.

You're right that it so happens that reduceStrength's use of PureCSE still results in a "maximal" CSE - every CSEable case will eventually be caught - because in those cases where a Value's owner is null, the reduceStrength will have set changed to true.  I thinks it's useful that you say this, but the algorithm would still have been correct otherwise since we already know that this is a super rare case.  Even if changed was false in this case, we would probably still use this fix.

I think that it's useful to spell out the contract that PureCSE now has: it will ignore Values with null owners, which even means that a client of PureCSE could deliberately null the owner if they wanted to signal that the value should be ignored.  I can imagine this being useful.

[Yusuke Suzuki]

Comment on attachment 299142 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:20
>> +        since `m_changed = true` is specified.
> 
> I think that the burden of proof for your change is much lower: you can ignore the value with owner = nullptr because:
> 
> 1. The value's memory is indeed still valid. The fact that the memory is valid permits us to load Value::owner and testing it is meaningful. In your new improved algorithm, it does not matter what the state of Values with null owners is so long as they are owned by Procedure (i.e. the value's memory is still valid). For example, it doesn't matter what the dominance situation of Values with null owners is.
> 
> 2. PureCSE is allowed to be conservative. The compiler does not depend on PureCSE to always succeed.
> 
> You're right that it so happens that reduceStrength's use of PureCSE still results in a "maximal" CSE - every CSEable case will eventually be caught - because in those cases where a Value's owner is null, the reduceStrength will have set changed to true.  I thinks it's useful that you say this, but the algorithm would still have been correct otherwise since we already know that this is a super rare case.  Even if changed was false in this case, we would probably still use this fix.
> 
> I think that it's useful to spell out the contract that PureCSE now has: it will ignore Values with null owners, which even means that a client of PureCSE could deliberately null the owner if they wanted to signal that the value should be ignored.  I can imagine this being useful.

Right! PureCSE now has the ability to ignore the substitution with Value which owner is nullptr.
And yup. We do not rely on the result of maximal CSE.
And since the changed Value still dominates all the same basic blocks which are previously dominated, CSE performed before clearning Value->owner is still valid. That is what I mean in (1).

[Yusuke Suzuki]

Created attachment 299231 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 299231 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:11873:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:11874:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:11874:  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/testb3.cpp:11875:  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.

[Yusuke Suzuki]

Committed r210919: <http://trac.webkit.org/changeset/210919>