Repository navigation
Conversation
| // Heap2Local cannot optimize this due to the overlapping live ranges (it sees | ||
| // the x.value operations are not exclusive to any single allocation). But this | ||
| // pass can optimize into this: |
There was a problem hiding this comment.
The reason Heap2Local can't handle this is because it only analyzes a single allocation at a time, rather than doing a single analysis phase covering all allocations in the function. This design limitation is also responsible for significant additional complexity in the pass. Have you experimented with fixing Heap2Local to analyze all allocations at once?
There was a problem hiding this comment.
Yes,
Handling live ranges, sort of like liveness of locals for register allocation / local coalescing, is possible, but quite complex
I had Gemini get this fully working, then simplify as much as it (and I) could, and it was... not pretty.
Basically you need to layer an additional LivenessAnalysis in Heap2Local, just adding a lot of complexity. Sadly it can't replace any existing part. It is even harder than a normal locals-based liveness because we must track allocations as they live in locals, but also as they flow - e.g. two allocations flowing through arms of the same If.
| return std::make_unique<PropagateStructGets>(); | ||
| } | ||
|
|
||
| // Track the number of gets for each local, so that we can see if all gets are |
There was a problem hiding this comment.
| // Track the number of gets for each local, so that we can see if all gets are | |
| // Track the number of gets for each local, so that we can see if all the local.gets are |
| // The sets for each local index. | ||
| std::unordered_map<Index, std::vector<Expression**>> sets; | ||
|
|
||
| // The parent struct.gets for each local index's gets. |
There was a problem hiding this comment.
| // The parent struct.gets for each local index's gets. | |
| // The parent struct.gets for each local index's local.gets. |
I want to keep the different gets as separate as possible :)
| // The parent struct.gets for each local index's gets. | ||
| std::unordered_map<Index, std::vector<StructGet*>> getParents; | ||
|
|
||
| // The parent struct.get for each tee. |
There was a problem hiding this comment.
It would be good to briefly say what happens when a local.get or local.tee does not have a parent struct.get.
| // Avoid unreachable code, and leave atomic operations for now TODO | ||
| if (curr->type == Type::unreachable || | ||
| curr->order != MemoryOrder::Unordered) { |
There was a problem hiding this comment.
There should be no need to leave atomic struct.gets for later. If the accessed allocations don't escape, they can't be part of any synchronization between threads, so they can be optimized just like non-atomic accesses.
There was a problem hiding this comment.
Also, if we can optimize structs that don't come from allocations, this would still be valid when all their fields are immutable. Immutable fields cannot participate in cross-thread synchronization.
| return; | ||
| } | ||
|
|
||
| auto* module = getModule(); |
There was a problem hiding this comment.
| auto* module = getModule(); | |
| auto* wasm = getModule(); |
Maybe there's some more nuanced pattern about where we use module vs wasm, but my impression is that we generally use wasm in passes, at least.
| if (teeIt == teeParents.end()) { | ||
| // This tee does not feed into a struct.get, so the allocation(s) | ||
| // might escape. | ||
| ok = false; | ||
| break; |
There was a problem hiding this comment.
This also could have been checked directly via an ExpressionStackWalker rather than after the fact.
| // Each used field's value must not change between the local.set and | ||
| // local.get: either the field is immutable, or every set writes a fresh | ||
| // struct.new that only ever flows into this local's struct.gets. (Note |
There was a problem hiding this comment.
How could the field's value change, given that we have ruled out struct.sets? Also, it is not clear which local.set and local.get this refers to.
There was a problem hiding this comment.
Gemini says we've only ruled out struct.sets through this particular local, but aliasing could allow other sets to slip through. It would be good to clarify that in the comment.
| // bar(x_value, x_value); | ||
| // | ||
| // Note how the `.value` (the struct.get) happens *before* `foo()`, | ||
| // after the optimization, so we may have moved a trap around here. It is |
There was a problem hiding this comment.
Don't we know the local.set values always come from allocations, though?
There was a problem hiding this comment.
Gemini says the pass also supports structs coming from parameters or calls as long as their fields are all immutable. It would be good to update the top-level comment to reflect that.
|
|
||
| void visitTry(Try* curr) { hasTry = true; } | ||
|
|
||
| void visitFunction(Function* func) { |
There was a problem hiding this comment.
It would be nice to break this up into helper functions. It's huge.
| ;; As above, but the immutable field has a non-nullable reference type | ||
| ;; ((ref struct)). The optimizeed local uses a nullable type (since it is | ||
| ;; not dominated in the spec's sense by sets), and we add ref.as_non_null | ||
| ;; on the tees and get so validation succeeds across blocks. |
Our main escape analysis pass, Heap2Local, does not handle overlapping
live ranges, things like this:
More than one allocation is being read from in those
x.valueoperations,so Heap2Local sees the uses are not "exclusive", and it gives up.
Handling live ranges, sort of like liveness of locals for register allocation /
local coalescing, is possible, but quite complex. However, the pattern
above can be optimized in another way, which this new pass handles:
when the reference is always read from (using
struct.get, which is the.valueoperations above), then we can just shift from using the referenceto using data:
We store the data in a new local
x_value, and that is all we need.That is, this pass handles a situation that is more restricted than
Heap2Local, in that it doesn't do complex analysis of control flow
and handling of
ref.eqand such, but it is more general in that itcan handle overlapping live ranges in a trivial way.
Fixes #8641 , a Dart testcase, and I saw this helps real-world binaries from
Kotlin and Java too - this is a common pattern.