Run closure specialization only if ownership info is present in callee #84956
Conversation
…back Calling `cloneRecursively` from `SpecializationInfo.cloneClosures` requires the pullback having ownership info. Otherwise, the cloner uses `recordFoldedValue` instead of `recordClonedInstruction`, and `postProcess` hook is not called, which leads to an assertion failure in `BridgedClonerImpl::cloneInst`. TODO: investigate why the assertion fails only on Linux. See: * swiftlang#84800 (comment) * swiftlang#84955
|
Tagging @JaapWijnen |
|
@swift-ci please smoke test |
| // TODO: investigate why the assertion fails only on Linux. See | ||
| // * https://github.com/swiftlang/swift/pull/84800#issuecomment-3398200989 | ||
| // * https://github.com/swiftlang/swift/issues/84955 | ||
| partialApply.referencedFunction?.hasOwnership == true |
There was a problem hiding this comment.
Can you put this check into isCalleeSpecializable, because we need to check it also in the regular closure specialization
There was a problem hiding this comment.
Can you put this check into
isCalleeSpecializable, because we need to check it also in the regular closure specialization
Applied the suggestion in 93169c1, thanks.
| // uses `recordFoldedValue` instead of `recordClonedInstruction`, and | ||
| // `postProcess` hook is not called, which leads to an assertion | ||
| // failure in `BridgedClonerImpl::cloneInst`. | ||
| // TODO: investigate why the assertion fails only on Linux. See |
There was a problem hiding this comment.
You can remove the TODO. It's definitely not working to specialize a non OSSA callee. We just might get lucky on macos
There was a problem hiding this comment.
You can remove the TODO. It's definitely not working to specialize a non OSSA callee. We just might get lucky on macos
@eeckstein Do I get you point correct that such a mismatch in ownership between the function containing apply site and the callee is actually a valid behavior? So, we do not need to investigate why the mismatch occurs, but we just need to use this additional check introduced in this PR - is this correct?
If so, I should probably also close the issue #84955 should probably be closed, right?
There was a problem hiding this comment.
Do I get you point correct that such a mismatch in ownership between the function containing apply site and the callee is actually a valid behavior?
Yes, currently. This is because the OwnershipModelEliminator is a function pass. And functions are processed bottom-up the call tree. Therefore it can happen that one function is still in OSSA while the other isn't anymore.
This is of course not ideal and I'm going to change the OwnershipModelEliminator to be a module pass (eeckstein@878f3ab on my private branch)
There was a problem hiding this comment.
You can remove the TODO. It's definitely not working to specialize a non OSSA callee. We just might get lucky on macos
@eeckstein Removed the TODO in 93169c1, thanks for suggestion
kovdan01
changed the title
|
@eeckstein I've also changed the PR description and title so it's no longer AutoDiff-specific |
|
@swift-ci please smoke test |
|
@swift-ci smoke test macos platform |
2 similar comments
|
@swift-ci smoke test macos platform |
|
@swift-ci smoke test macos platform |
swiftlang#84956) Calling `cloneRecursively` from `SpecializationInfo.cloneClosures` requires the callee having ownership info. Otherwise, the cloner uses `recordFoldedValue` instead of `recordClonedInstruction`, and `postProcess` hook is not called, which leads to an assertion failure in `BridgedClonerImpl::cloneInst`.
swiftlang#84956) Calling `cloneRecursively` from `SpecializationInfo.cloneClosures` requires the callee having ownership info. Otherwise, the cloner uses `recordFoldedValue` instead of `recordClonedInstruction`, and `postProcess` hook is not called, which leads to an assertion failure in `BridgedClonerImpl::cloneInst`.
4 participants
Calling
cloneRecursivelyfromSpecializationInfo.cloneClosuresrequires the callee having ownership info. Otherwise, the cloner usesrecordFoldedValueinstead ofrecordClonedInstruction, andpostProcesshook is not called, which leads to an assertion failure inBridgedClonerImpl::cloneInst.