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.
Created attachment 299142 [details] Patch
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.
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
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
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.
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).
Created attachment 299231 [details] Patch for landing
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.
Committed r210919: <http://trac.webkit.org/changeset/210919>