[ownership] Try harder to make sure we do not propagate ownership info when ownership is disabled. #34693

gottesmm · 2020-11-11T21:21:18Z

Specifically, I made it so that assuming our instruction is inserted into a
block already that we:

Return a constraint of {OwnershipKind::Any, UseLifetimeConstraint::NonLifetimeEnding}.
Return OwnershipKind::None for all values.

Noticed above I said that if the instruction is already inserted into a block
then we do this. The reason why is that if this is called before an instruction
is inserted into a block, we can't get access to the SILFunction that has the
information on whether or not we are in OSSA form. The only time this can happen
is if one is using these APIs from within SILBuilder since SILBuilder is the
only place where we allow this to happen. In SILBuilder, we already know whether
or not our function is in ossa or not and already does different things as
appropriate (namely in non-ossa does not call getOwnershipKind()). So we know
that if these APIs are called in such a situation, we will only be calling it if
we are in OSSA already. Given that, we just assume we are in OSSA if we do not
have a function.

To make sure that no mistakes are made as a result of that assumption, I put in
a verifier check that all values when ownership is disabled return a
OwnershipKind::None from getOwnershipKind().

The main upside to this is this means that we can write code for both
OSSA/non-OSSA and write code for non-None ownership without needing to check if
ownership is enabled.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

gottesmm · 2020-11-11T21:22:56Z

@atrick keep in mind that there is a weird behavior I haven't gotten to the bottom of but this seems to make basic.sil fail due to the SILModule not loading any SIL. It is the only place that I see this behavior and I don't understand why it is happening. I am going to talk with @CodaFi in a bit to figure out why.

atrick

A couple of minor comments for next PR cleanup

atrick · 2020-11-11T22:42:27Z

lib/SIL/Verifier/SILVerifier.cpp

@@ -1108,6 +1111,23 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
      // Make sure that if operand is generic that its primary archetypes match
      // the function context.
      checkLegalType(I->getFunction(), operand.get(), I);
+
+      // If we are not in OSSA, our operand constraint should be none if we have


"should be none if we have"...
replace with
"should be invalid for a type dependent operand" (or Optional::None)

atrick · 2020-11-11T22:44:58Z

lib/SIL/Verifier/SILVerifier.cpp

+      // UseLifetimeConstraint::NonLifetimeEnding.
+      if (!I->getFunction()->hasOwnership()) {
+        auto constraint = operand.getOwnershipConstraint();
+        require(operand.isTypeDependent() || constraint.hasValue(),


require(operand.isTypeDependent() || constraint.hasValue()
you can move this inside the if (!operand.isTypeDependent()) check

atrick · 2020-11-11T22:47:12Z

lib/SIL/IR/OperandOwnership.cpp

+  // We do not ever call this function when an instruction isn't in a block.
+  assert(getUser()->getParent() &&
+         "Can not lookup ownership constraint unless inserted into block");
+  if (!getUser()->getFunction()->hasOwnership())


But instructions are not always in a SILFunction!

Touche. I did it right below. Wasn't thinking!

gottesmm · 2020-11-11T23:38:37Z

@swift-ci smoke test

gottesmm · 2020-11-12T02:35:47Z

I found the problem with the basic.sil test. We were using an ownership instruction in non-ossa and were not emitting a diagnostic for some reason. I put in a backstop SILBasicBlock diagnostic to catch such things in the future here: #34699.

gottesmm · 2020-11-12T02:37:40Z

@swift-ci smoke test

…o when ownership is disabled. Specifically, I made it so that assuming our instruction is inserted into a block already that we: 1. Return a constraint of {OwnershipKind::Any, UseLifetimeConstraint::NonLifetimeEnding}. 2. Return OwnershipKind::None for all values. Noticed above I said that if the instruction is already inserted into a block then we do this. The reason why is that if this is called before an instruction is inserted into a block, we can't get access to the SILFunction that has the information on whether or not we are in OSSA form. The only time this can happen is if one is using these APIs from within SILBuilder since SILBuilder is the only place where we allow this to happen. In SILBuilder, we already know whether or not our function is in ossa or not and already does different things as appropriate (namely in non-ossa does not call getOwnershipKind()). So we know that if these APIs are called in such a situation, we will only be calling it if we are in OSSA already. Given that, we just assume we are in OSSA if we do not have a function. To make sure that no mistakes are made as a result of that assumption, I put in a verifier check that all values when ownership is disabled return a OwnershipKind::None from getOwnershipKind(). The main upside to this is this means that we can write code for both OSSA/non-OSSA and write code for non-None ownership without needing to check if ownership is enabled.

gottesmm · 2020-11-12T02:57:21Z

@swift-ci smoke test

In swiftlang#34693, we discovered the SIL parser can silently fail. Try to detect this in +asserts builds.

marcrasi · 2020-12-22T05:34:18Z

test/AutoDiff/SILOptimizer/differentiation_function_canonicalization.sil

-// CHECK:   strong_retain [[JVP_FN]]
 // CHECK:   [[VJP_FN:%.*]] = differentiable_function_extract [vjp] [[ARG]]
-// CHECK:   strong_retain [[VJP_FN]]


I think that these retains are actually necessary, because I'm seeing ASAN failures happening starting at this commit (https://bugs.swift.org/browse/SR-13973).

This commit stops emitting these changes because it makes the .getOwnershipKind() check at https://github.com/apple/swift/blob/d3d1f8d22d07233ae5714d810c26aff57066b453/lib/SILOptimizer/Mandatory/Differentiation.cpp#L503 & https://github.com/apple/swift/blob/d3d1f8d22d07233ae5714d810c26aff57066b453/lib/SILOptimizer/Mandatory/Differentiation.cpp#L1216 return None.

I don't really understand any of this, so I don't know what would be a good way to fix it. Is there some other check that we can do in Differentiation.cpp that does the right thing?

I did try completely deleting the check (so that Differentiation.cpp always emits the copy value operation), but that caused a lot of "Error! Found a leaked owned value that was never consumed." So I guess there are some cases where we do need a copy value and some cases where we don't need it, and I don't know the right way to distinguish between them :)

(There are also some other calls to getOwnershipKind() in Differentiation.cpp that might need to be updated, though I don't yet have any concrete examples of things that they break.)

gottesmm requested a review from atrick November 11, 2020 21:22

atrick approved these changes Nov 11, 2020

View reviewed changes

atrick reviewed Nov 11, 2020

View reviewed changes

gottesmm force-pushed the pr-1fdd1272490dcfe27f62410aee521669e0059430 branch from 096c390 to 816d75e Compare November 11, 2020 23:38

gottesmm force-pushed the pr-1fdd1272490dcfe27f62410aee521669e0059430 branch from 816d75e to 8681e0b Compare November 12, 2020 02:36

gottesmm force-pushed the pr-1fdd1272490dcfe27f62410aee521669e0059430 branch from 8681e0b to 58d4191 Compare November 12, 2020 02:57

CodaFi mentioned this pull request Nov 12, 2020

Add an Assert To Make Sure the SIL Parser Emits Diagnostics #34701

Merged

gottesmm merged commit 7d05f37 into swiftlang:main Nov 12, 2020

gottesmm deleted the pr-1fdd1272490dcfe27f62410aee521669e0059430 branch November 12, 2020 05:31

CodaFi added a commit to CodaFi/swift that referenced this pull request Nov 12, 2020

Add an Assert To Make Sure the SIL Parser Emits Diagnostics

c7cce10

In swiftlang#34693, we discovered the SIL parser can silently fail. Try to detect this in +asserts builds.

marcrasi reviewed Dec 22, 2020

View reviewed changes

marcrasi mentioned this pull request Dec 22, 2020

[AutoDiff] fix ownership instructions (SR-13973) #35196

Closed

marcrasi mannequin mentioned this pull request Dec 22, 2020

[SR-13973] valueWithPullback for a method: ASAN failure #56368

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ownership] Try harder to make sure we do not propagate ownership info when ownership is disabled. #34693

[ownership] Try harder to make sure we do not propagate ownership info when ownership is disabled. #34693

Uh oh!

gottesmm commented Nov 11, 2020

Uh oh!

gottesmm commented Nov 11, 2020

Uh oh!

atrick left a comment

Uh oh!

atrick Nov 11, 2020

Uh oh!

atrick Nov 11, 2020

Uh oh!

atrick Nov 11, 2020

Uh oh!

gottesmm Nov 11, 2020

Uh oh!

gottesmm commented Nov 11, 2020

Uh oh!

gottesmm commented Nov 12, 2020

Uh oh!

gottesmm commented Nov 12, 2020

Uh oh!

gottesmm commented Nov 12, 2020

Uh oh!

marcrasi Dec 22, 2020

Uh oh!

Uh oh!

[ownership] Try harder to make sure we do not propagate ownership info when ownership is disabled. #34693

[ownership] Try harder to make sure we do not propagate ownership info when ownership is disabled. #34693

Uh oh!

Conversation

gottesmm commented Nov 11, 2020

Uh oh!

gottesmm commented Nov 11, 2020

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

atrick Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

atrick Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

atrick Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Nov 11, 2020

Uh oh!

gottesmm commented Nov 12, 2020

Uh oh!

gottesmm commented Nov 12, 2020

Uh oh!

gottesmm commented Nov 12, 2020

Uh oh!

marcrasi Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!