-
Notifications
You must be signed in to change notification settings - Fork 10.5k
SILCombine: Constant-fold MemoryLayout<T>.offset(of: \.literalKeyPath) #32544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@swift-ci test |
@swift-ci benchmark |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -OnoneCode size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview
|
Build failed |
@swift-ci smoke test linux |
This is clever, but it relies on undefined behavior. 0 is not a valid value for a |
case KeyPathPatternComponent::Kind::OptionalChain: | ||
case KeyPathPatternComponent::Kind::OptionalForce: | ||
case KeyPathPatternComponent::Kind::OptionalWrap: | ||
hasOffset = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not try to optimize if the key path component refers to a resilient external property, because the property being referenced may in fact be stored. If component.getExternalDecl()
returns nonnull, we should leave the call in place. It'd be good to include a test for this, where a library-evolution-enabled module exports a struct, and we validate that key paths referencing the struct's stored properties from a client module still correctly return offsets after this optimization is passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point.
@jckarter Hm, I would prefer to keep that as a SIL optimization. How about using a Builtin.baseAddressForOffset instead of the 0-rawpointer (which is lowered to a null pointer in IRGen)? |
@eeckstein That could work. |
…ations. The ``base_addr_for_offset`` instruction creates a base address for offset calculations. The result can be used by address projections, like ``struct_element_addr``, which themselves return the offset of the projected fields. IR generation simply creates a null pointer for ``base_addr_for_offset``.
Replace a call of the getter of AnyKeyPath._storedInlineOffset with a "constant" offset, in case of a keypath literal. "Constant" offset means a series of struct_element_addr and tuple_element_addr instructions with a 0-pointer as base address. These instructions can then be lowered to "real" constants in IRGen for concrete types, or to metatype offset lookups for generic or resilient types. Replace: %kp = keypath ... %offset = apply %_storedInlineOffset_method(%kp) with: %zero = integer_literal $Builtin.Word, 0 %null_ptr = unchecked_trivial_bit_cast %zero to $Builtin.RawPointer %null_addr = pointer_to_address %null_ptr %projected_addr = struct_element_addr %null_addr ... // other address projections %offset_ptr = address_to_pointer %projected_addr %offset_builtin_int = unchecked_trivial_bit_cast %offset_ptr %offset_int = struct $Int (%offset_builtin_int) %offset = enum $Optional<Int>, #Optional.some!enumelt, %offset_int rdar://problem/53309403
@jckarter I pushed a new version |
@swift-ci smoke test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
Replace a call of the getter of AnyKeyPath._storedInlineOffset with a "constant" offset, in case of a keypath literal.
"Constant" offset means a series of struct_element_addr and tuple_element_addr instructions with a 0-pointer as base address.
These instructions can then be lowered to "real" constants in IRGen for concrete types, or to metatype offset lookups for generic or resilient types.
Replace:
with:
rdar://problem/53309403