[wasm] Inline immovable roots into JS->Wasm wrappers [v8/v8 : main]
Matthias Liedtke (Gerrit)
Matthias Liedtke added 1 comment![]( "Open in Gerrit")
Related details
- Daniel Lehmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke added 1 comment![]( "Open in Gerrit")
@paol...@microsoft.com: FYI, the idea is when we compile for a specific isolate (JS-to-wasm wrappers, both inlined and non-inlined), we can make better use of that by "constant-folding" some isolate-dependent data directly into the code (which is something that JS JIT code does everywhere).
Related details
- Daniel Lehmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann added 1 comment![]( "Open in Gerrit")
@paol...@microsoft.com: FYI, the idea is when we compile for a specific isolate (JS-to-wasm wrappers, both inlined and non-inlined), we can make better use of that by "constant-folding" some isolate-dependent data directly into the code (which is something that JS JIT code does everywhere).
Recording offline discussion: I compared the inlined wrappers in Turbofan vs. the inlined wrappers in Turboshaft vs. https://crrev.com/c/7224611 vs. this CL, and couldn't find this CL to improve over the fastpath change in the linked/base. See https://docs.google.com/spreadsheets/d/1KdqGIWqaCw8M5b5uS2-cA7LYkz4At6YToEj0p3ljE7A/edit?usp=sharing for numbers.
I'll leave this to Matthias, since I didn't look at the generated code.
Related details
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke added 1 comment![]( "Open in Gerrit")
Daniel Lehmann@paol...@microsoft.com: FYI, the idea is when we compile for a specific isolate (JS-to-wasm wrappers, both inlined and non-inlined), we can make better use of that by "constant-folding" some isolate-dependent data directly into the code (which is something that JS JIT code does everywhere).
Recording offline discussion: I compared the inlined wrappers in Turbofan vs. the inlined wrappers in Turboshaft vs. https://crrev.com/c/7224611 vs. this CL, and couldn't find this CL to improve over the fastpath change in the linked/base. See https://docs.google.com/spreadsheets/d/1KdqGIWqaCw8M5b5uS2-cA7LYkz4At6YToEj0p3ljE7A/edit?usp=sharing for numbers.
I'll leave this to Matthias, since I didn't look at the generated code.
Pasting my reply here as well:
OK, my optimization works, we replace:
> cmpl [r13+0x360] (root (heap_number_map)),rdx
which loads the heap number map from the root register with
> cmpl rdx,0x515
So I guess it doesn't have any impact because we have those awesome 4 chained loads that we have also analyzed before that probably make this one independent load completely unobservable.
Still, this is a nice improvement and we should go for it. I didn't claim that it causes measurable speed-ups, but it eliminates unnecessary loads. 🎉
(And the comparison is also 1 byte less code size 😋)
Related details
- Daniel Lehmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke added 1 comment![]( "Open in Gerrit")
Daniel Lehmann@paol...@microsoft.com: FYI, the idea is when we compile for a specific isolate (JS-to-wasm wrappers, both inlined and non-inlined), we can make better use of that by "constant-folding" some isolate-dependent data directly into the code (which is something that JS JIT code does everywhere).
Matthias LiedtkeRecording offline discussion: I compared the inlined wrappers in Turbofan vs. the inlined wrappers in Turboshaft vs. https://crrev.com/c/7224611 vs. this CL, and couldn't find this CL to improve over the fastpath change in the linked/base. See https://docs.google.com/spreadsheets/d/1KdqGIWqaCw8M5b5uS2-cA7LYkz4At6YToEj0p3ljE7A/edit?usp=sharing for numbers.
I'll leave this to Matthias, since I didn't look at the generated code.
Pasting my reply here as well:
OK, my optimization works, we replace:
> cmpl [r13+0x360] (root (heap_number_map)),rdxwhich loads the heap number map from the root register with
> cmpl rdx,0x515So I guess it doesn't have any impact because we have those awesome 4 chained loads that we have also analyzed before that probably make this one independent load completely unobservable.
Still, this is a nice improvement and we should go for it. I didn't claim that it causes measurable speed-ups, but it eliminates unnecessary loads. 🎉
(And the comparison is also 1 byte less code size 😋)
And just to clarify: This doesn't only affect the `HeapNumberMap` but all instances of immovable immortal roots like `null` or `undefined`.
Related details
- Daniel Lehmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke added 2 comments![]( "Open in Gerrit")
@mano...@chromium.org: As Daniel is out, could you please take a look? I'll be out starting next week, so I'd like to land this change before that. 😊
if (isolate != nullptr) {
Handle<Object> root = isolate->root_handle(index);
const bool is_smi = i::IsSmi(*root);
if constexpr (std::is_convertible_v<Smi, RootObjectType>) {
if (is_smi) {
return assembler.SmiConstant(Cast<Smi>(*root));
}
}
CHECK(!is_smi);
return assembler.HeapConstantMaybeHole(i::Cast<RootObjectType>(root));
}This is the semantic change.
Related details
- Daniel Lehmann
- Manos Koukoutos
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manos Koukoutos (Gerrit)
Manos Koukoutos voted and added 1 comment![]( "Open in Gerrit")
Votes added by Manos Koukoutos
| Code-Review | +1 |
1 comment
if (isolate != nullptr) {
Handle<Object> root = isolate->root_handle(index);
const bool is_smi = i::IsSmi(*root);
if constexpr (std::is_convertible_v<Smi, RootObjectType>) {
if (is_smi) {
return assembler.SmiConstant(Cast<Smi>(*root));
}
}
CHECK(!is_smi);
return assembler.HeapConstantMaybeHole(i::Cast<RootObjectType>(root));
}This is the semantic change.
- Daniel Lehmann
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manos Koukoutos (Gerrit)
Manos Koukoutos added 1 comment![]( "Open in Gerrit")
#include <type_traits>Shouldn't this be with the other includes?
Related details
- Daniel Lehmann
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 2 comments![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Commit-Queue | +2 |
2 comments
Shouldn't this be with the other includes?
Oops, yes, I might have used the "quick-fix" button to add the include automatically... 😞
Related details
- Daniel Lehmann
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/compiler/turboshaft/wasm-assembler-helpers.h
Insertions: 2, Deletions: 1.
@@ -5,11 +5,12 @@
#ifndef V8_COMPILER_TURBOSHAFT_WASM_ASSEMBLER_HELPERS_H_
#define V8_COMPILER_TURBOSHAFT_WASM_ASSEMBLER_HELPERS_H_
-#include <type_traits>
#if !V8_ENABLE_WEBASSEMBLY
#error This header should only be included if WebAssembly is enabled.
#endif // !V8_ENABLE_WEBASSEMBLY
+#include <type_traits>
+
#include "src/compiler/turboshaft/operations.h"
#include "src/roots/roots.h"
```
Change information
[wasm] Inline immovable roots into JS->Wasm wrappers
This will e.g. inline the HeapNumber map into the wrapper instead of
loading it each time via the root register.
This change also replaces the magic macro with a template function,
e.g. instead of
> LOAD_ROOT(null_string)
where `null_string` is just some random text, it uses
> __ LoadRootWasm<RootIndex::knull_string>()
which uses a proper symbol `RootIndex::knull_string` to refer to the
actual root allowing both code navigation as well as auto-completion.
- M src/compiler/turboshaft/assembler.h
- M src/compiler/turboshaft/wasm-assembler-helpers.h
- M src/compiler/turboshaft/wasm-lowering-reducer.h
- M src/wasm/turboshaft-graph-interface.cc
- M src/wasm/wrappers-inl.h
- M src/wasm/wrappers.h
Code-Review: +1 by Manos Koukoutos
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |