[AutoDiff] Fix several issues related to captured arguments #41401
Conversation
|
Tagging @dan-zheng @BradLarson Note that the first fix is essentially a workaround mentioned in https://bugs.swift.org/browse/SR-15205. Much better fix would be teaching |
|
CC: @rxwei |
|
@swift-ci please test |
test/AutoDiff/compiler_crashers_fixed/sr15205-diff-capture.swift
Outdated
Show resolved
Hide resolved
|
|
||
| if (silParameterIndices->getCapacity() < parameterIndices->getCapacity()) { | ||
| assert(original->getCaptureInfo().hasLocalCaptures()); | ||
| silParameterIndices = | ||
| silParameterIndices->extendingCapacity(original->getASTContext(), | ||
| parameterIndices->getCapacity()); | ||
| } | ||
|
|
There was a problem hiding this comment.
How about adding a numCaptures parameter to autodiff::getLoweredParameterIndices and make sure we allocate the right capacity in the first place?
There was a problem hiding this comment.
There are few workarounds of similar kind sprinkled over the code, so we'd need to eliminate them as well (see https://github.com/apple/swift/blob/main/lib/SILOptimizer/Mandatory/Differentiation.cpp#L518 as an example). Just setting numCaptures seems not work in general as we need to know how the captured arguments will be lowered.
There was a problem hiding this comment.
Yeah it's not a perfect solution. Since captured arguments are always at the end of a SIL function, we could get the number of captures from CaptureInfo. Tuple captures won't be splat in SIL arguments, so we might be able to assume the number of captures in CaptureInfo is the number of captures in the SIL function, but I'm not sure entirely.
There was a problem hiding this comment.
Me too :) I'm planning to resolve the TODO afterwards, after checking all possibilities and fixing getLoweredParameterIndices properly. There is a tentative patch for this, but it needs more testing and checking.
There was a problem hiding this comment.
LGTM as is since similar workarounds already exist, but it would be really nice to fix getLoweredParameterIndices. Let me know if you want to take a stab at that or rather merge this first.
- Introduce a workaround while dealing with getLoweredParameterIndices() results:
it operates on AST and therefore does not take into account captured arguments
which are "on side" on AST and explicit in SIL
- Ensure captured arguments (@inout_aliased) are handled in a same way as ordinary
@inout arguments while generating a pullback type
Fixes SR-15205
|
@swift-ci please test |
|
Any chance this will resolve SR-15793? |
|
No. Note this is fixing a compiler crash, not a runtime result. |
|
Thank you @asl ! |
I thought these were related because my bug is caused by the parameter index being off by 1. This PR fixes a bug with parameter indices. Still, this should be a good starting point for launching an investigation into SR-15793. |
it operates on AST and therefore does not take into account captured arguments
which are "on side" on AST and explicit in SIL
@inout_aliased) are handled in a same way as ordinary@inoutarguments while generating the pullback typeResolves SR-15205