Give each GC struct field and array element its own alias region - #14351
Conversation
| let access = if is_exn { | ||
| GcAccess::ExnPayload { | ||
| exn_ty: struct_ty, | ||
| field, | ||
| } | ||
| } else { | ||
| GcAccess::StructField { | ||
| ty: struct_ty, | ||
| field, | ||
| } | ||
| }; |
There was a problem hiding this comment.
With the current split/interning of types, is this split necessary? I'd have thought that struct_ty is already unique across exceptions/structs of the same structure. For example above the internal composite_type field is already different across exceptions/structs.
There was a problem hiding this comment.
For structs we need to walk up the subtyping chain to find the type that introduced the field. For exceptions, there is no subtyping chain to walk. This split lets us funnel each type to the appropriate code path. I suppose we could try to unify them, and exceptions would just stop after one iteration of the subtype chain walk, since they never have a supertype, but I don't think we really gain anything from doing that other than removing just this single if, but then also we'd muddy the water a bit for exactly what we're accessing (which we also aren't gaining a ton from right now, admittedly, but I do appreciate not losing precision when there is no reason to, so I think changing anything here is a bit of a wash on either side, so I'd ultimately prefer to leave it as-is unless you feel strongly about this).
There was a problem hiding this comment.
Definitely don't feel strong, it just felt weird to single out exceptions here when the desired properties otherwise fall out of not specializing around them.
| /// retain internal assertion metadata to report if such a trap happens. This | ||
| /// is here to ensure that in the face of heap corruption that there's no | ||
| /// possible UB within Cranelift and/or the runtime. | ||
| fn gc_memflags(&mut self, func: &mut ir::Function) -> ir::MemFlagsData { |
There was a problem hiding this comment.
Perhaps rename this to gc_header_memflags to avoid mis-use?
| /// An access of one of an array's elements, by the type at the access site. | ||
| ArrayElements { ty: ModuleInternedTypeIndex }, |
There was a problem hiding this comment.
Is it worth it trying to perhaps future-proof against the https://github.com/WebAssembly/multibyte-array-access proposal here? In that world I think we'll have to canonical all arrays-of-scalars to one GcAccess region, right?
We probably won't implement that proposal for some time, but if we were to implement that in ~6 months I'd be worried that we wouldn't have any reason to come back and handle this.
There was a problem hiding this comment.
Is it worth it trying to perhaps future-proof against the https://github.com/WebAssembly/multibyte-array-access proposal here? In that world I think we'll have to canonical all arrays-of-scalars to one
GcAccessregion, right?
I don't believe so, because we will still know the type of the array object even if we are accessing its elements as u64 instead of u8 or whatever, and we choose the alias region based on the array object's type, not the array's elements' type: with this PR today, we already use different alias regions for two different (array i8) (as long as they really are two different types in two different subtyping hierarchies after canonicalization, e.g. due to rec groups). So we should already be future proof, afaik.
We have to take care to name the same region for a field that is shared between sub- and supertypes. This is a ~0.3% speed up (in terms of instructions retired; my laptop is too noisy to get stable wall time numbers) on execution for the GC-using sightglass benchmarks. Note that we don't do dead-store elimination for GC object accesses because they are marked as trapping accesses, so this is just enabling more redundant-load elimination and store-to-load forwarding. This is also (surprisingly) a ~2-3% speed up for compile times for the GC-using sightglass benchmarks, presumably because we eliminate more loads and therefore have less instructions to process through the rest of compilation. This commit does not introduce alias regions for the different fields inside a GC object header or the array length; that is left for follow up commits.
0c5f68d to
a63f13e
Compare
2 participants
We have to take care to name the same region for a field that is shared between sub- and supertypes.
This is a ~0.3% speed up (in terms of instructions retired; my laptop is too noisy to get stable wall time numbers) on execution for the GC-using sightglass benchmarks. Note that we don't do dead-store elimination for GC object accesses because they are marked as trapping accesses, so this is just enabling more redundant-load elimination and store-to-load forwarding.
This is also (surprisingly) a ~2-3% speed up for compile times for the GC-using sightglass benchmarks, presumably because we eliminate more loads and therefore have less instructions to process through the rest of compilation.
This commit does not introduce alias regions for the different fields inside a GC object header or the array length; that is left for follow up commits.