A couple of SIL fixes #34267
A couple of SIL fixes #34267
Conversation
…rojections My upcoming SILGen change introduces more stores directly into the result of a ref_element_addr or struct_element_addr in some cases. Make sure we handle the failing combinations flagged by the ownership verifier.
DI had trouble with this pattern: %s = struct_element_addr ... %t0 = tuple_element_addr %s, 0 %t1 = tuple_element_addr %s, 1 %f = function_ref ... apply %f(%t0, %t1) This is because the NonLoadUses map only stored a single use of a tuple element per instruction, preventing instructions such as 'apply' from being able to use multiple tuple elements. In other cases where this comes up, such as with 'assign' and 'copy_addr', DI scalarizes the tuple operation by projecting each component, however clearly this can't be easily done with an 'apply'. Instead, we can get DI out of the business of scalarization, at least for instructions which are known to perform an unconditional initialization. We do this by changing the NonLoadUses map to store a vector of DIMemoryUse IDs instead of a single value.
All the tests seem to pass without it now.
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please benchmark |
Performance: -OCode size: -OPerformance: -OsizeCode size: -OsizePerformance: -Onone
Code 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 |
| isa<InitExistentialAddrInst>(user) || | ||
| isa<InitEnumDataAddrInst>(user) || isa<BeginAccessInst>(user) || | ||
| isa<TailAddrInst>(user) || isa<IndexAddrInst>(user) || | ||
| isa<UnconditionalCheckedCastAddrInst>(user)) { |
There was a problem hiding this comment.
@slavapestov This is incorrect. UnconditionalCheckedCastAddrInst does not have any results. Instead, you need to select its uses directly. Can you add an error test for unconditional_checked_cast_addr as well just to show via a test that we are correct here?
| %2 = alloc_box $<τ_0_0> { var S<τ_0_0> } <T>, var, name "self" | ||
| %3 = mark_uninitialized [rootself] %2 : $<τ_0_0> { var S<τ_0_0> } <T> | ||
| %4 = project_box %3 : $<τ_0_0> { var S<τ_0_0> } <T>, 0 | ||
| %5 = struct_element_addr %4 : $*S<T>, #S.x |
There was a problem hiding this comment.
@slavapestov This has far fewer tests than the old version that was deleted above. Are you sure we didn't lose any code coverage?
The ownership verifier got confused by code where a stored property of a class was initialized in-place with an enum value, or was the destination of a cast, erroneously flagging an ownership violation:
DI got confused by code where more than one component of tuple value was initialized in-place via out parameters of an
applyinstruction, erroneously diagnosing that all but the first tuple component were uninitialized:I guess neither code pattern is generated anywhere in the ownership SIL pipeline today, but I imagine it will come up once we more aggressively eliminate
copy_addrsbefore lowering ownership. Also, a change that I'm working on refactors some SILGen code to fix a bug, and the refactoring makes use of in-place initialization, which now generates the above code patterns instead of initializing a temporary and then performing acopy_addrfrom the temporary.Finally, DI had some code to 'scalarize' certain tuple operations. After fixing the dataflow analysis to cope with instructions that have multiple non-load uses (such as an
applythat initializes both components of a tuple), it appears that we no longer need this step at all.