1034 // Turn this: BitAnd(value, all-ones) 1035 // Into this: value. 1036 if ((m_value->type() == Int64 && m_value->child(1)->isInt(std::numeric_limits<uint64_t>::max())) 1037 || (m_value->type() == Int32 && m_value->child(1)->isInt(std::numeric_limits<uint32_t>::max()))) { 1038 replaceWithIdentity(m_value->child(0)); 1039 break; 1040 } B3::Value::isInt is 262 inline bool Value::isInt(int64_t value) const 263 { 264 return hasInt() && asInt() == value; 265 } And B3::Value::asInt is 257 inline int64_t Value::asInt() const 258 { 259 return hasInt32() ? asInt32() : asInt64(); 260 } So, UINT32_MAX will become `static_cast<int64_t>(UINT32_MAX)` (not -1), and comparing with -1 in `asInt() == value`, and false!
Created attachment 378446 [details] Patch
Created attachment 378447 [details] Patch
Comment on attachment 378447 [details] Patch r=me
Committed r249721: <https://trac.webkit.org/changeset/249721>
<rdar://problem/55226605>
Comment on attachment 378447 [details] Patch nice
Shows 16% JetStream2/pdfjs improvement on iPhoneX. One macOS bot finishes running JetStream2 now and it is also reporting 32% JetStream2/pdfjs improvement. On the other hand, macOS bot is saying this causes 21% JetStream2/mandreel regression. However, iOS bot is saying neutral. I would like to collect more data points since it would be noise.
(In reply to Yusuke Suzuki from comment #7) > Shows 16% JetStream2/pdfjs improvement on iPhoneX. > One macOS bot finishes running JetStream2 now and it is also reporting 32% > JetStream2/pdfjs improvement. > > On the other hand, macOS bot is saying this causes 21% JetStream2/mandreel > regression. > However, iOS bot is saying neutral. > > I would like to collect more data points since it would be noise. I think this patch is causing some wrong behavior. I'm now investigating.
Comment on attachment 378447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378447&action=review > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1209 > Value* bitAnd = m_insertionSet.insert<Value>(m_index, BitAnd, m_value->origin(), m_value->child(0)->child(0), m_value->child(1)->bitXorConstant(m_proc, m_value->child(0)->child(1))); Found the existing issue. This `m_value->child(1)->bitXorConstant(m_proc, m_value->child(0)->child(1))` is not inserted. We can get crash with validate-graph
Re-opened since this is blocked by bug 201667
Created attachment 378512 [details] Patch
Created attachment 378513 [details] Patch
Comment on attachment 378513 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378513&action=review > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1135 > + Value* newConstant = m_value->child(1)->bitXorConstant(m_proc, m_value->child(0)->child(1)); > + ASSERT(newConstant); > + m_insertionSet.insertValue(m_index, newConstant); > + Value* bitOr = m_insertionSet.insert<Value>(m_index, BitOr, m_value->origin(), m_value->child(0)->child(0), newConstant); This optimization is never exercised previously. This patch fixed the bug, and this patch revealed the above existing issue, `newConstant` is not inserted.
Created attachment 378514 [details] Patch
Comment on attachment 378514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378514&action=review > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1135 > + Value* newConstant = m_value->child(1)->bitXorConstant(m_proc, m_value->child(0)->child(1)); > + ASSERT(newConstant); > + m_insertionSet.insertValue(m_index, newConstant); > + Value* bitOr = m_insertionSet.insert<Value>(m_index, BitOr, m_value->origin(), m_value->child(0)->child(0), newConstant); Found this bug and fixed. > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1215 > + && m_value->child(0)->child(1)->isInt32(std::numeric_limits<uint32_t>::max())))) { > + Value* newConstant = m_value->child(1)->bitXorConstant(m_proc, m_value->child(0)->child(1)); > + ASSERT(newConstant); > + m_insertionSet.insertValue(m_index, newConstant); > + Value* bitAnd = m_insertionSet.insert<Value>(m_index, BitAnd, m_value->origin(), m_value->child(0)->child(0), newConstant); Ditto.
Comment on attachment 378514 [details] Patch r=me
On my iMac Pro, 1. pdfjs improvement is because the previous patch broke it. 2. mandreel issue is not revealed on my iMac Pro 3. async-fs shows ~10% improvement since its Math.random has many `and 0xffffffff, value` and they are swept now correctly. My original intention of this patch is improving async-fs since I found crazy and operations in async-fs, and it seems that this patch works well.
(In reply to Robin Morisset from comment #16) > Comment on attachment 378514 [details] > Patch > > r=me Thanks!
Committed r249747: <https://trac.webkit.org/changeset/249747>