[wasm-wide-arith] Implement add128 for ia32 architecture in Turboshaft [v8/v8 : main]
Ryan Diaz (Gerrit)
Ryan Diaz added 1 comment![]( "Open in Gerrit")
uint64_t* ptr = reinterpret_cast<uint64_t*>(data);My research seemed to indicate that I can't use a `__int128_t` because it isn't accessible when compiling for 32 bit architectures, but not sure if that is true.
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 3 comments![]( "Open in Gerrit")
Votes added by Clemens Backes
| Code-Review | +1 |
3 comments
constexpr int kSlotSize = MemoryRepresentation::Uint64().SizeInBytes();Why not just `kInt64Size`?
__ StackSlot(kSlotSize * 4, kSlotSize);As discussed over mail, this can create huge stack frames because the stack memory is not shared. That's maybe something to fix more generally in Turboshaft, but we should keep it in mind.
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 3 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
3 comments
LGTM with comments.
__ StackSlot(kSlotSize * 4, kSlotSize);As discussed over mail, this can create huge stack frames because the stack memory is not shared. That's maybe something to fix more generally in Turboshaft, but we should keep it in mind.
I'd be fine with leaving this as is for now and improving it in a follow-up, but I think it's so easy that we might as well do it right away: look at `parent_frame_state_` for precedent of an IR node that's cached on the graph builder. Introduce a similar `OptionalV<WordPtr> wide_ops_stack_buffer_`, wrap it in an accessor `GetWideOpsStackBuffer() { ... }` that creates it on demand and returns it as non-optional `V<WordPtr>`, and that should already be enough to do the trick. I'd hard-code `static constexpr int kWideOpsStackBufferSize = 4 * kInt64Size`, and then add a `static_assert(k…Size >= 4 * kSlotSize)` guard here just to set a precedent for possible future ops that might want to reuse that buffer and need other sizes.
uint64_t* ptr = reinterpret_cast<uint64_t*>(data);My research seemed to indicate that I can't use a `__int128_t` because it isn't accessible when compiling for 32 bit architectures, but not sure if that is true.
Oh, bummer.
Please add a comment, roughly `We need this in particular for 32-bit platforms, where __uint128_t is not available.`
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")
constexpr int kSlotSize = MemoryRepresentation::Uint64().SizeInBytes();Why not just `kInt64Size`?
yes, I forgot that constant existed :) using that now
Jakob KummerowAs discussed over mail, this can create huge stack frames because the stack memory is not shared. That's maybe something to fix more generally in Turboshaft, but we should keep it in mind.
I'd be fine with leaving this as is for now and improving it in a follow-up, but I think it's so easy that we might as well do it right away: look at `parent_frame_state_` for precedent of an IR node that's cached on the graph builder. Introduce a similar `OptionalV<WordPtr> wide_ops_stack_buffer_`, wrap it in an accessor `GetWideOpsStackBuffer() { ... }` that creates it on demand and returns it as non-optional `V<WordPtr>`, and that should already be enough to do the trick. I'd hard-code `static constexpr int kWideOpsStackBufferSize = 4 * kInt64Size`, and then add a `static_assert(k…Size >= 4 * kSlotSize)` guard here just to set a precedent for possible future ops that might want to reuse that buffer and need other sizes.
Thank you for the explanation. Makes a lot of sense I was hesitant to add an instance variable for this, but seems like it was always going to be necessary. Done.
Jakob KummerowMy research seemed to indicate that I can't use a `__int128_t` because it isn't accessible when compiling for 32 bit architectures, but not sure if that is true.
Oh, bummer.
Please add a comment, roughly `We need this in particular for 32-bit platforms, where __uint128_t is not available.`
- Clemens Backes
- Jakob Kummerow
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 Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
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. |