Repository navigation
Conversation
| // 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.
There was a problem hiding this comment.
We do optimize cases where structs arrive from e.g. parameters, i.e. not allocations that we view. See e.g. the test $set-unknown-immutable. Yes, that depends on immutability, as you said.
Is that enough for atomic synchronization? Would the struct.gets need to be atomic?
There was a problem hiding this comment.
Yes, we can still optimize in that case. No, the struct.gets do not need to be atomic.
There was a problem hiding this comment.
Is that because a Publish must happen after the immutable field is written at creation?
Added optimizing of atomic operations + tests.
There was a problem hiding this comment.
No, it's because cross-thread synchronization requires a release (or stronger) write on one thread that is then read from by an acquire (or stronger) read on another thread. The first half of that cannot happen with an immutable field. The initializing write done by struct.new and friends is not a release write.
There was a problem hiding this comment.
So other threads can read immutable fields non-atomically? Immutable writes (i.e. creations) are guaranteed to synchronize without atomics?
There was a problem hiding this comment.
Yes, that pattern is safe due to the publish built into struct.new. But the synchronization effect of the publish does not extend to anything besides the fields of the particular published struct.
Co-authored-by: Thomas Lively <tlively123@gmail.com>
Co-authored-by: Thomas Lively <tlively123@gmail.com>
| // | ||
| // If we replace `l = x` with `l_value = x.value` on the first line, then | ||
| // we need it to remain the same until the return at the end. If x can be | ||
| // written to, that is a danger. Note how this cannot happen if each set |
| ;; H2L-NEXT: ) | ||
| (func $struct.get-atomic-param (param $x i32) (param $p (ref $struct)) (result i32) | ||
| (local $ref (ref null $struct)) | ||
| ;; Now we do not see one set, which writes a param. But the field is |
There was a problem hiding this comment.
"do not see one set" => "have something other than an allocation flow into a local.set"
(and in the next test)
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.