Simplify scratch local calculation #6583
Conversation
Change `countScratchLocals` to return the count and type of necessary scratch locals. It used to encode them as keys in the global map from scratch local types to local indices, which could not handle having more than one scratch local of a given type and was generally harder to reason about due to its use of global state. Take the opportunity to avoid emitting unnecessary scratch locals for `TupleExtract` expressions that will be optimized to not use them. Also simplify and better document the calculation of the mapping from IR indices to binary indices for all locals, scratch and non-scratch.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
src/wasm/wasm-stack.cpp
Outdated
| scratchLocals[localType] = index - 1; | ||
| ScratchLocalFinder(BinaryInstWriter& parent) : parent(parent) {} | ||
|
|
||
| // We need two i32 scratch locals for reach string slice, but they can be |
There was a problem hiding this comment.
| // We need two i32 scratch locals for reach string slice, but they can be | |
| // We need two i32 scratch locals for each string slice, but they can be |
| ;; ROUNDTRIP-NEXT: (local $15 (tuple i32 f32)) | ||
| ;; ROUNDTRIP-NEXT: (local $16 i32) | ||
| ;; ROUNDTRIP-NEXT: (local $14 (tuple i32 f32)) | ||
| ;; ROUNDTRIP-NEXT: (local $15 i32) |
There was a problem hiding this comment.
The comment on line 80 looks like it needs to be updated.
There was a problem hiding this comment.
So is the scratch local created when first reading the binary, (local $14 f32) now unnecessary? Is the reason because this was optimized by extractedGets? (It looks before we were counting extractedGets as scratch locals anyway)
In any case the comment needs to be updated from line 78.
There was a problem hiding this comment.
Yes, good point, I think we no longer emit (local $14). I updated the comments.
src/wasm/wasm-stack.cpp
Outdated
|
|
||
| // We need two i32 scratch locals for reach string slice, but they can be | ||
| // reused. | ||
| bool hasStringSlice = false; |
There was a problem hiding this comment.
Nope, this is left over development cruft, will remove.
Change
countScratchLocalsto return the count and type of necessary scratchlocals. It used to encode them as keys in the global map from scratch local
types to local indices, which could not handle having more than one scratch
local of a given type and was generally harder to reason about due to its use of
global state. Take the opportunity to avoid emitting unnecessary scratch locals
for
TupleExtractexpressions that will be optimized to not use them.Also simplify and better document the calculation of the mapping from IR indices
to binary indices for all locals, scratch and non-scratch.