Heap2Local: Optimize Arrays in addition to Structs #6478
Conversation
src/passes/Heap2Local.cpp
Outdated
| if (auto* arrayNew = allocation->dynCast<ArrayNew>()) { | ||
| numFields = getIndex(arrayNew->size); | ||
| } else if (auto* arrayNewFixed = allocation->dynCast<ArrayNewFixed>()) { | ||
| numFields = arrayNewFixed->values.size(); |
There was a problem hiding this comment.
Maybe this part can be factored into a getSize helper?
| } else if (auto* arrayNewFixed = allocation->dynCast<ArrayNewFixed>()) { | ||
| // Simply use the same values as the array. | ||
| structNew = builder.makeStructNew(structType, arrayNewFixed->values); | ||
| arrayNewReplacement = structNew; |
There was a problem hiding this comment.
Do we need to noteIsReached in this case as well? If not, why? If it's because of the noteCurrentIsReached calls below, then why do we need the noteIsReached above?
There was a problem hiding this comment.
Ah, good point, this was not consistent. It happened to work but it's confusing. I moved the noteIsReached to a shared location and applied it to both structNew and arrayNewReplacement uniformly, with a better (hopefully) explanation.
src/passes/Heap2Local.cpp
Outdated
| if (reached->type == nullArray) { | ||
| reached->type = nullStruct; | ||
| } else if (reached->type == nonNullArray) { | ||
| reached->type = nonNullStruct; | ||
| } | ||
| } |
There was a problem hiding this comment.
What about supertypes of the array type?
There was a problem hiding this comment.
Good catch, the fuzzer noticed this overnight as well 😄 Fixed.
| noteCurrentIsReached(); | ||
| } | ||
|
|
||
| void visitArrayGet(ArrayGet* curr) { |
There was a problem hiding this comment.
Do we need to handle other array operations like ArrayCopy, ArrayFill, or the string allocation instructions?
There was a problem hiding this comment.
We have a TODO for those. This is safe atm as EscapeAnalyzer will assume the worst for anything it does not recognize, like those.
| ;; CHECK-NEXT: (drop | ||
| ;; CHECK-NEXT: (ref.null none) | ||
| ;; CHECK-NEXT: ) |
There was a problem hiding this comment.
It might be worth checking side effects when preserving the reference just to reduce test verbosity 🤔
There was a problem hiding this comment.
Yeah, sorry about the verbosity. This pass has not make an effort to check effects so far - not sure it's worth changing that, as it would add more code complexity.
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
| (func $array.flowing.type (result i32) | ||
| (local $temp (ref $array)) |
There was a problem hiding this comment.
Does this still work properly when the local is a supertype of the allocated array type?
There was a problem hiding this comment.
Yes. We had a test for that with structs and I added one for arrays now, good idea.
test/lit/passes/heap2local.wast
Outdated
| ;; Arrays with reference values. | ||
| (module | ||
| ;; CHECK: (type $array (sub (array (ref null $array)))) | ||
| (type $array (sub (array (ref null $array)))) |
There was a problem hiding this comment.
Is it important that this type is sub? If not, perhaps we can elide that part.
There was a problem hiding this comment.
Sounds good, I removed that part.
To keep things simple, this adds a Array2Struct component to the pass. When we
find a non-escaping array, we run that to turn it into a struct, and then run the
existing Struct2Local to convert that to locals. This avoids refactoring Struct2Local
to handle both structs and arrays (with the downside of making the optimization of
arrays a little less efficient, but they are rarer, I suspect - that is certainly the case
in Java output I've seen).
The core EscapeAnalyzer logic is generalized to handle both arrays and structs,
but the changes there are thankfully quite minor.