[webkit-reviews] review canceled: [Bug 194081] B3 should use associativity to optimize expression trees : [Attachment 366294] Patch

[bugzilla-daemon at webkit.org]

Robin Morisset <rmorisset at apple.com> has canceled Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 194081: B3 should use associativity to optimize expression trees
https://bugs.webkit.org/show_bug.cgi?id=194081

Attachment 366294: Patch

https://bugs.webkit.org/attachment.cgi?id=366294&action=review




--- Comment #13 from Robin Morisset <rmorisset at apple.com> ---
Comment on attachment 366294
  --> https://bugs.webkit.org/attachment.cgi?id=366294
Patch

I've got the performance numbers from this patch on JetStream2 and they're
overall neutral.
So I looked in more detail at when this optimization fires in a non-trivial way
on all of JetStream2, and there are only a few subtests where it triggers:
- Unipoker: balances some BitOr trees (insignificant slowdown)
- tsf-wasm: balances a ton of BitOr trees (insignificant 1.4% speedup)
- stanford-crypto-sha256 and stanford-crypto-pbkdf2: balances a few BitXor
trees (insignificant slowdown on first iteration, strongly significant 2%
speedup on average iteration and 3.5% speedup on worst iteration)
- octane-zlib: balances a couple huge Add trees (no change)
- ML: balances one small BitXor tree (no change)
- mandreel: balances a couple (Add/BitAnd) small trees (0.7% insignificant
slowdown on first iteration, strongly significant 1% improvement on average
iteration and 2% on worst-case iteration)
- HashSet-wasm: balances a few BitOr trees (insignificant speedup)
- base64-SP: only balances one tiny BitXor tree (significant 2% slowdown on
average iteration(?), insignificant change otherwise)

So at best we get a couple percent on three benchmarks, and are otherwise
neutral.
Most likely we are just seeing noise even in these benchmarks where it balances
trees. Surprisingly (to me) it does not find simplification opportunities
beyond simple balancing.

I thus put this patch out-of-review, as it does not seem obviously worth it at
this point. I plan to revisit it once we have better value analysis in B3. It
should enable significantly more simplifications, and make more large trees
appear by turning CheckAdd/Sub/Mul into their non-checked variants.