[wasm-wide-arith] Implement Wide Multiplication on ia32 and arm in Liftoff [v8/v8 : main]
Ryan Diaz (Gerrit)
Ryan Diaz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ryan Diaz
| Commit-Queue | +1 |
2 comments
inline void EmitI64MulWide(LiftoffAssembler* assm, bool is_signed) {Ryan DiazWould it be easier to call out to C (is there some library function we could use)?
And to which extend is this covered by tests? Would now be a good time to add some fuzzing support, maybe something similar to the `Float32CrossCompilerDeterminismTest`?
Ryan DiazYou mean do a similar thing to what we did for 32 bit add/sub128 https://chromium-review.git.corp.google.com/c/v8/v8/+/7797503 ?
Yes, I think that is simpler, I wasn't sure if it was better / necessary, but I'll go ahead and add external ref function for this.
For tests, I'm relying on the js unittests here: https://source.chromium.org/chromium/chromium/src/+/main:v8/test/mjsunit/wasm/wide-arithmetic.js;l=1?q=test%2Fmjsunit%2Fwasm%2Fwide-arithmetic.js&sq=&ss=chromium%2Fchromium%2Fsrc:v8%2F
I agree there should be more robust coverage, I'll look into the test you mentioned.
Ok updated this CL to us an external call for liftoff wide mul for both 32 bit architectures.
for (int i = 1; i <= 2; ++i) {
VarState& slot = __ cache_state() -> stack_state.end()[-i];
if (slot.is_const()) {
__ Spill(slot.offset(), slot.constant());
slot.MakeStack();
}
}This was the suggestion for materializing constants from the bug related to add/sub128 potential memory read errors. I will separately go apply this to those ops and pull it out into a separate function in a separate cl.
Related details
- Clemens Backes
- Jakob Kummerow
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes voted and added 5 comments![]( "Open in Gerrit")
Votes added by Clemens Backes
| Code-Review | +1 |
5 comments
if (slot.is_const()) {
__ Spill(slot.offset(), slot.constant());
slot.MakeStack();
}It would be slightly simpler to just call `Spill(&slot);` unconditionally.
for (int i = 1; i <= 2; ++i) {
VarState& slot = __ cache_state() -> stack_state.end()[-i];
if (slot.is_const()) {
__ Spill(slot.offset(), slot.constant());
slot.MakeStack();
}
}This was the suggestion for materializing constants from the bug related to add/sub128 potential memory read errors. I will separately go apply this to those ops and pull it out into a separate function in a separate cl.
Ack.
#if V8_TARGET_ARCH_X64 || V8_TARGET_ARCH_ARM64 || V8_TARGET_ARCH_IA32 || \See https://crrev.com/c/7847572: We should fully remove these `#if`s from the function body decoder. This can happen in any order (it's fine to extend the list here and then drop it in a separate CL).
void wasm_int64_mul_wide_s_wrapper(Address data) {To save a bit of binary size and to avoid accidentally calling this on 64-bit architectures, we could have a `#if V8_TARGET_ARCH_64_BIT \n UNREACHABLE() \n #else \n ...` (same below of course).
We could also go a step further and not even *define* the external references on 64-bit systems. Feel free to try that if you like. But it's optional.
Related details
- Jakob Kummerow
- Ryan Diaz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
1 comment
- Ryan Diaz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
Hello,
A number of macro guards have been removed by this CL: http://crrev.com/c/7847572
PPC/S390 don't yet support wide ops and we will have them implemented in the near future.
Would it be safer to add the guards back for said platforms or just skip the `mjsunit/wasm/wide-arithmetic` tests for now?
Related details
- Ryan Diaz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
Hello,
A number of macro guards have been removed by this CL: http://crrev.com/c/7847572
PPC/S390 don't yet support wide ops and we will have them implemented in the near future.
Would it be safer to add the guards back for said platforms or just skip the `mjsunit/wasm/wide-arithmetic` tests for now?
If you add back some bailouts, do that at some lower level, ideally in the platform-specific `LiftoffAssembler` methods. If it's just failing tests and not failing compilation, you can also just skip. Whatever is easier for you.
Related details
- Milad Farazmand
- Ryan Diaz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
Clemens BackesHello,
A number of macro guards have been removed by this CL: http://crrev.com/c/7847572
PPC/S390 don't yet support wide ops and we will have them implemented in the near future.
Would it be safer to add the guards back for said platforms or just skip the `mjsunit/wasm/wide-arithmetic` tests for now?
If you add back some bailouts, do that at some lower level, ideally in the platform-specific `LiftoffAssembler` methods. If it's just failing tests and not failing compilation, you can also just skip. Whatever is easier for you.
It will hit the `unimplemented` blocks in liftoff. Do you expect more tests will get added soon which use this feature (given it's experimental) or `wide-arithmetic` is the only test for now? trying to prioritize this item.
Related details
- Ryan Diaz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow added 1 comment![]( "Open in Gerrit")
Clemens BackesHello,
A number of macro guards have been removed by this CL: http://crrev.com/c/7847572
PPC/S390 don't yet support wide ops and we will have them implemented in the near future.
Would it be safer to add the guards back for said platforms or just skip the `mjsunit/wasm/wide-arithmetic` tests for now?
Milad FarazmandIf you add back some bailouts, do that at some lower level, ideally in the platform-specific `LiftoffAssembler` methods. If it's just failing tests and not failing compilation, you can also just skip. Whatever is easier for you.
It will hit the `unimplemented` blocks in liftoff. Do you expect more tests will get added soon which use this feature (given it's experimental) or `wide-arithmetic` is the only test for now? trying to prioritize this item.
"experimental" in this case just means that it's a feature we're currently implementing; it's not ready for production yet but we're working on getting it into that state. The upstream spec is pretty far along ("phase 3"); I'd expect this to ship by default not too long from now.
Related details
- Milad Farazmand
- Ryan Diaz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
Clemens BackesHello,
A number of macro guards have been removed by this CL: http://crrev.com/c/7847572
PPC/S390 don't yet support wide ops and we will have them implemented in the near future.
Would it be safer to add the guards back for said platforms or just skip the `mjsunit/wasm/wide-arithmetic` tests for now?
Milad FarazmandIf you add back some bailouts, do that at some lower level, ideally in the platform-specific `LiftoffAssembler` methods. If it's just failing tests and not failing compilation, you can also just skip. Whatever is easier for you.
Jakob KummerowIt will hit the `unimplemented` blocks in liftoff. Do you expect more tests will get added soon which use this feature (given it's experimental) or `wide-arithmetic` is the only test for now? trying to prioritize this item.
"experimental" in this case just means that it's a feature we're currently implementing; it's not ready for production yet but we're working on getting it into that state. The upstream spec is pretty far along ("phase 3"); I'd expect this to ship by default not too long from now.
Thank yo for the information, I'll skip them for now until we implement the ops: http://crrev.com/c/7856124
Related details
- Ryan Diaz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Diaz (Gerrit)
Ryan Diaz added 3 comments![]( "Open in Gerrit")
if (slot.is_const()) {
__ Spill(slot.offset(), slot.constant());
slot.MakeStack();
}It would be slightly simpler to just call `Spill(&slot);` unconditionally.
refactored to use `SpillLoopArgs` which does what you suggested.
#if V8_TARGET_ARCH_X64 || V8_TARGET_ARCH_ARM64 || V8_TARGET_ARCH_IA32 || \See https://crrev.com/c/7847572: We should fully remove these `#if`s from the function body decoder. This can happen in any order (it's fine to extend the list here and then drop it in a separate CL).
Done
void wasm_int64_mul_wide_s_wrapper(Address data) {To save a bit of binary size and to avoid accidentally calling this on 64-bit architectures, we could have a `#if V8_TARGET_ARCH_64_BIT \n UNREACHABLE() \n #else \n ...` (same below of course).
We could also go a step further and not even *define* the external references on 64-bit systems. Feel free to try that if you like. But it's optional.
went with the simpler `#if V8_TARGET_ARCH_64_BIT...` approach here
Related details
- Clemens Backes
- Jakob Kummerow
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Diaz (Gerrit)
Ryan Diaz added 1 comment![]( "Open in Gerrit")
void wasm_int64_mul_wide_s_wrapper(Address data) {Ryan DiazTo save a bit of binary size and to avoid accidentally calling this on 64-bit architectures, we could have a `#if V8_TARGET_ARCH_64_BIT \n UNREACHABLE() \n #else \n ...` (same below of course).
We could also go a step further and not even *define* the external references on 64-bit systems. Feel free to try that if you like. But it's optional.
went with the simpler `#if V8_TARGET_ARCH_64_BIT...` approach here
Done
Ryan Diaz (Gerrit)
Ryan Diaz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ryan Diaz
| Auto-Submit | +1 |
1 comment
inline void EmitI64MulWide(LiftoffAssembler* assm, bool is_signed) {Ryan DiazWould it be easier to call out to C (is there some library function we could use)?
And to which extend is this covered by tests? Would now be a good time to add some fuzzing support, maybe something similar to the `Float32CrossCompilerDeterminismTest`?
Ryan DiazYou mean do a similar thing to what we did for 32 bit add/sub128 https://chromium-review.git.corp.google.com/c/v8/v8/+/7797503 ?
Yes, I think that is simpler, I wasn't sure if it was better / necessary, but I'll go ahead and add external ref function for this.
For tests, I'm relying on the js unittests here: https://source.chromium.org/chromium/chromium/src/+/main:v8/test/mjsunit/wasm/wide-arithmetic.js;l=1?q=test%2Fmjsunit%2Fwasm%2Fwide-arithmetic.js&sq=&ss=chromium%2Fchromium%2Fsrc:v8%2F
I agree there should be more robust coverage, I'll look into the test you mentioned.
Ok updated this CL to us an external call for liftoff wide mul for both 32 bit architectures.
Done
Related details
- Clemens Backes
- Jakob Kummerow
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes voted and added 1 comment![]( "Open in Gerrit")
Votes added by Clemens Backes
| Code-Review | +1 |
1 comment
__ SpillLoopArgs(2);This could use a little comment, in particular since it is used outside of a loop.
Related details
- Jakob Kummerow
- Ryan Diaz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted Code-Review+1![]( "Open in Gerrit")
- Ryan Diaz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Diaz (Gerrit)
Ryan Diaz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ryan Diaz
| Auto-Submit | +1 |
| Code-Review | +1 |
1 comment
This could use a little comment, in particular since it is used outside of a loop.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Diaz (Gerrit)
Ryan Diaz voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
v8-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
v8-s...@luci-project-accounts.iam.gserviceaccount.com submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
11 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/wasm/baseline/liftoff-compiler.cc
Insertions: 2, Deletions: 0.
@@ -3105,7 +3105,9 @@
UNREACHABLE();
}
#elif V8_TARGET_ARCH_IA32 || V8_TARGET_ARCH_ARM
+ // Spill the two 64-bit arguments to multiplication.
__ SpillLoopArgs(2);
+ // Spill the remaining registers (if any).
__ SpillAllRegisters();
// Compute the lowest address of the two 64bit inputs.
```
Change information
[wasm-wide-arith] Implement Wide Multiplication on ia32 and arm in Liftoff
Use an external call to complete the 64x64 -> 128 bit multiplication ops
on 32 bit architectures in Liftoff.
- M src/codegen/external-reference.cc
- M src/codegen/external-reference.h
- M src/wasm/baseline/liftoff-compiler.cc
- M src/wasm/wasm-external-refs.cc
- M src/wasm/wasm-external-refs.h
Code-Review: +0 by Ryan Diaz, +1 by Clemens Backes, +1 by Jakob Kummerow
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |