Make Uncompress(Compress(ptr)) into a no-op. [v8/v8 : main]
Anton Bikineev (Gerrit)
Anton Bikineev voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anton Bikineev
| Code-Review | +1 |
1 comment
lgtm, thanks
For the context, we tried it in the past pre C++23 and it didn't work. At the time __builtin_assume__ was known to have issues that led to missing optimizations: https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609. I hope it's been fixed since then.
Related details
- Steinar H Gunderson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Steinar H Gunderson
| Commit-Queue | +2 |
1 comment
lgtm, thanks
For the context, we tried it in the past pre C++23 and it didn't work. At the time __builtin_assume__ was known to have issues that led to missing optimizations: https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609. I hope it's been fixed since then.
As far as I know, the issues are exactly the same still,, and I believe [[assume]] is just a different syntax for exactly the same thing. I prototyped this in Compiler Explorer with __builtin_assume until I could get it to work reliably (which was nontrivial, as you discovered), then changed to [[assume]] because it's cleaner.
In other words, one could in theory see regressions with this, also around inlining heuristics. But it appears to be a win size-wise, no regression CPU-wise, and makes generally sense to me. Thus, mlippautz@ asked me to send it to you :-)
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Steinar H Gunderson
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
+mlippautz for second V8 committer
Related details
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
- Steinar H Gunderson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Make Uncompress(Compress(ptr)) into a no-op.
Add an [[assume]] annotation so that Clang understands this; it doesn't
by itself (because it requires information about high bits in the
uncompressed pointer that we've found it hard to teach it).
Doesn't give a Speedometer win by itself, but it reduces the APK size
of 64-bit Chrome by ~11 kB.
- M include/cppgc/internal/member-storage.h
Code-Review: +1 by Michael Lippautz, +1 by Anton Bikineev
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
fwiw we had to remove a similar optimisation in V8's heap, because of UB when the assumption was invalidated (which it could be because of sandbox corruption): https://chromium-review.googlesource.com/c/v8/v8/+/6402513. Probably not an issue here but worth being aware of.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |