Summary: | [JSC] 32bit bitwide operation with all-one (-1) is wrong in B3 | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 201667 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2019-09-09 23:50:06 PDT
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> 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> |