WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
201634
[JSC] 32bit bitwide operation with all-one (-1) is wrong in B3
https://bugs.webkit.org/show_bug.cgi?id=201634
Summary
[JSC] 32bit bitwide operation with all-one (-1) is wrong in B3
Yusuke Suzuki
Reported
2019-09-09 23:50:06 PDT
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!
Attachments
Patch
(9.19 KB, patch)
2019-09-10 01:08 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(24.56 KB, patch)
2019-09-10 01:58 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(25.31 KB, patch)
2019-09-10 17:27 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(25.44 KB, patch)
2019-09-10 17:29 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(25.81 KB, patch)
2019-09-10 17:38 PDT
,
Yusuke Suzuki
rmorisset
: review+
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1
2019-09-10 01:08:58 PDT
Created
attachment 378446
[details]
Patch
Yusuke Suzuki
Comment 2
2019-09-10 01:58:58 PDT
Created
attachment 378447
[details]
Patch
Mark Lam
Comment 3
2019-09-10 07:45:22 PDT
Comment on
attachment 378447
[details]
Patch r=me
Yusuke Suzuki
Comment 4
2019-09-10 10:07:29 PDT
Committed
r249721
: <
https://trac.webkit.org/changeset/249721
>
Radar WebKit Bug Importer
Comment 5
2019-09-10 10:08:18 PDT
<
rdar://problem/55226605
>
Saam Barati
Comment 6
2019-09-10 11:52:48 PDT
Comment on
attachment 378447
[details]
Patch nice
Yusuke Suzuki
Comment 7
2019-09-10 16:32:22 PDT
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.
Yusuke Suzuki
Comment 8
2019-09-10 16:50:14 PDT
(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.
Yusuke Suzuki
Comment 9
2019-09-10 17:06:46 PDT
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
WebKit Commit Bot
Comment 10
2019-09-10 17:08:47 PDT
Re-opened since this is blocked by
bug 201667
Yusuke Suzuki
Comment 11
2019-09-10 17:27:21 PDT
Created
attachment 378512
[details]
Patch
Yusuke Suzuki
Comment 12
2019-09-10 17:29:01 PDT
Created
attachment 378513
[details]
Patch
Yusuke Suzuki
Comment 13
2019-09-10 17:30:14 PDT
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.
Yusuke Suzuki
Comment 14
2019-09-10 17:38:26 PDT
Created
attachment 378514
[details]
Patch
Yusuke Suzuki
Comment 15
2019-09-10 17:39:31 PDT
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.
Robin Morisset
Comment 16
2019-09-10 17:42:11 PDT
Comment on
attachment 378514
[details]
Patch r=me
Yusuke Suzuki
Comment 17
2019-09-10 17:44:21 PDT
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.
Yusuke Suzuki
Comment 18
2019-09-10 17:45:26 PDT
(In reply to Robin Morisset from
comment #16
)
> Comment on
attachment 378514
[details]
> Patch > > r=me
Thanks!
Yusuke Suzuki
Comment 19
2019-09-10 17:46:37 PDT
Committed
r249747
: <
https://trac.webkit.org/changeset/249747
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug