Bug 201634 – [JSC] 32bit bitwide operation with all-one (-1) is wrong in B3

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

[Yusuke Suzuki]

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!

[Yusuke Suzuki]

Created attachment 378446 [details]
Patch

[Yusuke Suzuki]

Created attachment 378447 [details]
Patch

[Mark Lam]

Comment on attachment 378447 [details]
Patch

r=me

[Yusuke Suzuki]

Committed r249721: <https://trac.webkit.org/changeset/249721>

[Radar WebKit Bug Importer]

<rdar://problem/55226605>

[Saam Barati]

Comment on attachment 378447 [details]
Patch

nice

[Yusuke Suzuki]

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]

(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 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]

Re-opened since this is blocked by bug 201667

[Yusuke Suzuki]

Created attachment 378512 [details]
Patch

[Yusuke Suzuki]

Created attachment 378513 [details]
Patch

[Yusuke Suzuki]

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]

Created attachment 378514 [details]
Patch

[Yusuke Suzuki]

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 on attachment 378514 [details]
Patch

r=me

[Yusuke Suzuki]

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]

(In reply to Robin Morisset from comment #16)
> Comment on attachment 378514 [details]
> Patch
> 
> r=me

Thanks!

[Yusuke Suzuki]

Committed r249747: <https://trac.webkit.org/changeset/249747>