LangOptions: Change default for RequirementMachineAbstractSignatures from Verify to Enabled #42111
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…from Verify to Enabled We've only seen false positives from verification here, and this request doesn't emit diagnostics.
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test macOS |
|
@slavapestov this PR fixed a major bug impacting Swift for TensorFlow, which had forced me to disable large chunks of the code base (s4tf/s4tf#6). I could have evaded the bug by passing As I investigated it, the stack trace changed from what's in the JIRA report. Before your #41631, the error message printed out a massive map of the generic constraints. The last stack frames were: Afterward, the stack trace ended in the following. The change was because of your modification to how Also, the error message was much more comprehensible. It said that RequirementMachine and GenericSignatureBuilder disagreed on something. SR-15884 was a very peculiar edge case that involved P.S. I'm reading your research paper right now, so I might understand what happened better afterward. I'm trying to fix SR-14228, another AutoDiff crasher, and I can't understand how to solve the bug without first knowing how RequirementMachine works. |
|
@philipturner Does RecurrentLayerCell inherit from CaseIterable? The purpose of verify mode was to catch requirement machine bugs -- a change in the minimized signature indicates a possible ABI break. However it sometimes caught GSB bugs as well, in particular the GSB would often drop requirements with signatures involving CaseIterable because it was unable to reason about the 'Self.Element == Self' requirement in the CaseIterable protocol. |
|
Here is the full reproducer. There is no CaseIterable, but it synthesizes something possibly similar: KeyPathIterable. import _Differentiation
@_spi(Reflection) import Swift
struct RNNCellInput<Input>: Differentiable {}
struct RNNCellOutput<Output>: Differentiable {}
protocol Layer: Differentiable {
associatedtype Input
associatedtype Output: Differentiable
@differentiable(reverse)
func callAsFunction(_ input: Input) -> Output
}
public protocol KeyPathIterable {
associatedtype AllKeyPaths: Sequence
where AllKeyPaths.Element == PartialKeyPath<Self>
}
protocol RecurrentLayerCell: Layer, KeyPathIterable
where
Input == RNNCellInput<TimeStepInput>,
Output == RNNCellOutput<TimeStepOutput>
{
associatedtype TimeStepInput
associatedtype TimeStepOutput: Differentiable
}
struct RecurrentLayer<Cell: RecurrentLayerCell>: Layer {
typealias Input = Cell.TimeStepInput
typealias Output = Cell.TimeStepOutput
var cell: Cell
@differentiable(reverse)
func callAsFunction(_ inputs: Cell.TimeStepInput) -> Cell.TimeStepOutput {
return self(inputs)
}
}Is it possible to narrow down this reproducer further, to use CaseIterable instead of KeyPathIterable, or another combination that triggers the same crash? |
|
It's a Reproducerimport _Differentiation
struct RNNCellInput<Input>: Differentiable {}
struct RNNCellOutput<Output>: Differentiable {}
protocol Layer: Differentiable {
associatedtype Input
associatedtype Output: Differentiable
@differentiable(reverse)
func callAsFunction(_ input: Input) -> Output
}
protocol RecurrentLayerCell: Layer, CaseIterable
where
Input == RNNCellInput<TimeStepInput>,
Output == RNNCellOutput<TimeStepOutput>
{
associatedtype TimeStepInput
associatedtype TimeStepOutput: Differentiable
}
struct RecurrentLayer<Cell: RecurrentLayerCell>: Layer {
typealias Input = Cell.TimeStepInput
typealias Output = Cell.TimeStepOutput
var cell: Cell
@differentiable(reverse)
func callAsFunction(_ inputs: Cell.TimeStepInput) -> Cell.TimeStepOutput {
return self(inputs)
}
}Mentioning #58149 |
|
What's the crash? |
|
I tested it on the March 30 toolchain. Crash logThat's the same as when it was RequirementMachineAbstractSignatures=Verify. Should I still add a regression test to the AutoDiff compiler crashers, or should I just close #58149? |
|
This is a bug in the GenericSignatureBuilder which is now disabled, and 'verify' mode is no longer on by default since we've finished qualifying the requirement machine. Please try with a newer developer snapshot. |
|
The newer (April 4) snapshot had no crash because it was on Enabled. I tried that toolchain first, then had to try again with the March 30 toolchain to reproduce the crash. The February 25 toolchain, which was before #41631, also has the oldest crash signature: Crash log |
|
That sounds fine then -- the GenericSignatureBuilder is deprecated and will soon be removed anyway. |
|
I just want to be 100% sure I'm understanding your response correctly. It is fine to add a regression test for SR-15884 to apple/swift, correct? |
|
Yeah, sure! |
Just to correct myself, I wasn't able to avoid the GSB bug by passing When I remove the SR-15884 workaround from S4TF, there will be no way it can compile on these older toolchains. I can at least set up a |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We've only seen false positives from verification here, and this request
doesn't emit diagnostics.