Repository navigation
Lower constant array.new_data in OptimizeInstructions - #9248
Conversation
array.new_data is not a constant expression, which blocks optimizations such as hoisting array literals into immutable globals, and passes like Precompute, Heap2Local, GUFA and SimplifyGlobals don't understand it. When the offset and size are constant and in bounds of a passive segment, lower it to: * array.new_default, if all elements are zero * array.new, if all elements are equal * array.new_fixed, if there are at most 64 elements Non-empty slices are only lowered with --traps-never-happen, since array.new_data traps if the segment was dropped. An empty slice at offset 0 can never trap, so it is always lowered. The 64-element limit exists because array.new_fixed costs a full *.const per element, while a data segment stores raw bytes. On large J2Wasm applications this removes over 99.7% of passive data segments and reduces binary size by ~1.8% (~1.3% gzipped).
| const auto& element = heapType.getArray().element; | ||
| if (!element.type.isNumber()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is this guarding against i31? (But I think that works too?)
There was a problem hiding this comment.
Actually per
binaryen/src/wasm/wasm-validator.cpp
Line 3894 in 0edef64
Would you prefer an assert or should I just drop it?
There was a problem hiding this comment.
Hmm, let's just drop this, that option seems best.
|
|
||
| uint64_t offset = offsetConst->value.getUnsigned(); | ||
| uint64_t size = sizeConst->value.getUnsigned(); | ||
| uint64_t elemBytes = element.getByteSize(); |
There was a problem hiding this comment.
These uint64_ts can be Address.
There was a problem hiding this comment.
Done. Would you like me to also use std::ckd_add and remove the comment?
| if (first == Literal::makeZero(element.type)) { | ||
| replaceCurrent(builder.makeArrayNew(heapType, sizeExpr)); | ||
| } else { | ||
| // visitArrayNew turns the size == 1 case into array.new_fixed. |
There was a problem hiding this comment.
It also handles a default value, I think, so first == ..zero.. is not needed here?
There was a problem hiding this comment.
Current implementation of makeArrayNew leaves code to be dropped (for the init value) which requires later Vacuum cleanup.
Maybe I remove this and then update visitArrayNew to only emit the drop when init has side effects. WDYT?
There was a problem hiding this comment.
In general it is simpler to leave cleanup for Vacuum. We usually check side effects when it enables an optimization.
Is there a particular reason you are concerned about the Vacuum here?
There was a problem hiding this comment.
No strong reason. I was just trying to avoid the extra step for optimization.
I suggested the visitArrayNew part as agent suggested couple of other places that did similarly (via getDroppedChildrenAndAppend) so I thought maybe you would prefer that approach.
Anyway I'm dropping the case here and won't update visitArrayNew. I will make sure lit results match.
| // stores raw bytes, so only do this for small arrays. Empirically, 64 sits | ||
| // in a natural gap between small/medium array literals (2D array rows, | ||
| // enum/switch tables, etc.) and large bulk lookup tables (100+ elements). | ||
| constexpr uint64_t MaxArrayNewFixedSize = 64; |
There was a problem hiding this comment.
How about putting this behind options.shrinkLevel == 0 (i.e. not doing it in -Os/-Oz)?
There was a problem hiding this comment.
Though, even so, 64 elements is a large increase in size. How likely is it that it gets optimized later?
There was a problem hiding this comment.
For CalcWorker, this was actually the good cut off for the smallest size. The main goal here is to make these constant expressions so they can be used for global initialization which later enables further optimizations (in our case esp. clinit constant hoisting taking advantage of that).
But that might not be true for all apps. If you like, we can put it behind options.shrinkLevel == 0 if you prefer or make the number itself configurable.
There was a problem hiding this comment.
Yeah, I am worried about the generalization to other codebases. But I don't have a clear intuition here...
Looks like Kotlin and Dart also use ArrayNewData. On some large binaries I saw no difference on Dart (no new optimization was possible), while on Kotlin this regressed size, with this diff:
[globals] : 13090
[imports] : 568
[memories] : 1
- [memory-data] : 1326742
+ [memory-data] : 1325066
[tables] : 0
[tags] : 1
- [total] : 1337835
+ [total] : 1338412
[vars] : 33597
ArrayCopy : 17
ArrayGet : 2494
ArrayLen : 950
ArrayNew : 97
- ArrayNewData : 48
- ArrayNewFixed : 435
+ ArrayNewData : 23
+ ArrayNewFixed : 460
ArraySet : 917
Binary : 34293
Block : 27894
Break : 8450
Call : 80567
CallRef : 7548
- Const : 143375
+ Const : 143952
Drop : 6746
GlobalGet : 32620
GlobalSet : 2941 Looks like half the ArrayNewDatas were optimized out, into new and larger ArrayNewFixed which could not be optimized out.
There was a problem hiding this comment.
Overall this added 786 bytes, but that is not significant in a 5 MB binary.
There was a problem hiding this comment.
I think we should have a high threshold here, then add a late-running pass that goes the other way and replaces large array.new_fixed with array.new_data/elem as a size optimization. Accepting the small size regression for some producers in the meantime seems fine.
There was a problem hiding this comment.
Just note that if produces are not aware of that late pass, they will get a hit. Anyway I already starting experimenting with the late pass tonight; I need to work on what a good number here is.
Interesting that the j2cl pass is needed. I wonder if we should add a general deduplication pass (maybe that is what you mean here?)
Particular j2cl pass I was referring to is the one that moves things out of to be eager globals. The deduplication is a separate optimization (that's part of the my large report that I shared earlier)
There was a problem hiding this comment.
Sounds good then, to keep the behavior in this PR, and consider the late deduplication pass. Now that we have a "late" stage that is safer than before, too.
There was a problem hiding this comment.
Ok will do.
(Note that, I assume there was a typo though; the late pass will not be a late deduplication pass but the reverse conversion pass)
There was a problem hiding this comment.
Yes, I meant some kind of reverse conversion (it would have the effect of deduplicating bytes in code into a single copy in data).
|
|
||
| uint64_t end; | ||
| if (std::ckd_add(&end, offset, size * elemBytes) || end > seg.data.size()) { | ||
| // offset and size are u32 and elemBytes <= 16, so this can't overflow. |
There was a problem hiding this comment.
offset and size are uint64_t as defined above. Is this referring to the fact that the wasm spec only allows i32s here? If so then I think it is better to keep the code more general (in case the spec allows i64).
…omMemory(bytes), defer default to visitArrayNew
| // matter; sign-extending gives the shorter encoding (-1 vs 255). | ||
| return Literal::makeFromMemory(src, uint8_t(elemBytes)); | ||
| } | ||
| return Literal::makeFromMemory(src, element.type); |
There was a problem hiding this comment.
Why do we need this if? Can't we always run line 2404, which has the right size?
There was a problem hiding this comment.
Oh, is the issue that we need the type to handle floats? Then how about adding the byteSize as an optional param to the old form of makeFromMemory?
| // (1 or 2) can be given to read a sign-extended packed value. A byteSize of | ||
| // 0 means the full size of the type. | ||
| static Literal | ||
| makeFromMemory(const void* p, Type type, unsigned byteSize = 0); |
There was a problem hiding this comment.
Sorry for the nitpick but please make this
makeFromMemory(const void* p, Type type, std::optional<unsigned> byteSize);There was a problem hiding this comment.
(this is the convention we use more often elsewhere)
There was a problem hiding this comment.
Updated. Pls let me know if it doesn't match the convention.
kripken
left a comment
There was a problem hiding this comment.
Otherwise please run the fuzzer for a bit to rule out any crashes, and lgtm
I ran it for 30 mins, about 6.6k iterations. |
Follow-up to #9248: a late pass that converts `array.new_fixed` of constant numeric values in function bodies to `array.new_data` when that is smaller (`array.new_fixed` needs a full `*.const` per element, a data segment stores raw bytes). Since `array.new_fixed` is a constant expression that other optimizations understand, the default pipeline runs this only in the last set of opts (`lastOpts`, #9238). * Global initializers are left alone, as `array.new_data` is not a constant expression there. * Converted payloads go into one new passive segment. Identical payloads are stored once, and a payload identical to an existing, never-dropped passive segment reuses that segment. * A group of identical payloads is converted only if the estimated binary size is smaller, and the new segment is only added if the savings that depend on it pay for its header. * The segment is never dropped and all reads are in bounds, so the result cannot trap. Note that this does not make it safe to raise the 64-element cap in OptimizeInstructions: the conversion is not always reversible. A larger `array.new_fixed` can be hoisted into a global initializer, where it cannot be turned back into `array.new_data`. On a large J2Wasm binary, removing the cap moved the remaining big tables into globals and grew the binary (+0.9 KB raw, +1.4 KB gzipped), which this pass cannot recover.
array.new_data is not a constant expression, which blocks optimizations
such as hoisting array literals into immutable globals, and passes like
Precompute, Heap2Local, GUFA and SimplifyGlobals don't understand it.
When the offset and size are constant and in bounds of a passive
segment, lower it to:
Non-empty slices are only lowered with --traps-never-happen, since
array.new_data traps if the segment was dropped. An empty slice at
offset 0 can never trap, so it is always lowered.
The 64-element limit exists because array.new_fixed costs a full
*.const per element, while a data segment stores raw bytes. On large
J2Wasm applications this removes over 99.7% of passive data segments and
reduces binary size by ~1.8% (~1.3% gzipped).