[AutoDiff] lift samefile derivative constriant

[marcrasi]

Lifts the samefile derivative constraint, and adds many testcases for cross-file and cross-module @derivative attrs.

The tests revealed many broken things. I filed TODOs for some of the broken things, and I fixed others in this PR.

The things that are fixed in this PR are:

• If there is no @differentiable attribute, then the differentiability witness must have the linkage of the derivative function. (Otherwise, when you define a @derivative of a function in a separate file or module, the differentiability witness definition gets external linkage because the original function has external linkage, and you're not supposed to give external linkage to definitions.)
• Need to check that derivative visibility is less than or equal to original function visibility because the opposite doesn't make sense.
• TBDGen needed to be changed back to operate on attributes rather than on AFD->getDerivativeFunctionConfigurations(), because a @derivative of a function in a different module puts a configuration on the function in the different module, and the TBDGenVisitor doesn't visit functions in other modules!
• Typechecking needed to be modified to eagerly check all the @derivative attributes in the module (rather than just the @derivative attributes in the primary file), so that the differentiation pass sees configurations arising from @derivatives in other files. This approach is slightly inspired by bindExtensions.

[rxwei]

@usableFromInline isn't necessary if we are registering an internal derivative, but is necessary if we are registering a public derivative.

[marcrasi]

Ok, this is ready for review now.

I'm running swift-apis and swift-models tests on this too because it does some pretty dramatic things.

[dan-zheng]

remove debug print statement

[marcrasi]

done

[rxwei]

For a public original function, how do we currently differentiate (no pun) between an internal derivative and a public derivative? Is that by the @usableFromInline attribute?

[dan-zheng]

Rather than adding an invasive diagnoseErrors flag, can you try using DiagnosticTransaction for diagnoseErrors = false users instead?

DiagnosticTransaction collects all diagnostics and provides a way to cancel them:

        DiagnosticTransaction transaction(Ctx.Diags);
        SWIFT_DEFER { transaction.abort(); };

See #28717 for an example.

[marcrasi]

I'll try this.

[marcrasi]

done

[marcrasi]

For a public original function, how do we currently differentiate (no pun) between an internal derivative and a public derivative? Is that by the @usableFromInline attribute?

Here are some current working behaviors:

// public derivative
@differentiable
public func f1(...) {...}
@derivative(of: f1)
internal func df1(...) {...}

// public derivative
public func f2(...) {...}
@derivative(of: f2)
public func df2(...) {...}

// internal derivative
public func f3(...) {...}
@derivative(of: f3)
internal func df3(...) {...}

[dan-zheng]

I believe AttributeChecker is just a thin wrapper around an ASTContext and we can call ASTContext::diagnose instead. How about removing the AttributeChecker * argument?

If it's possible to use DiagnosticTransaction in callers to abort diagnostics instead of conditionally emitting diagnostics based on a flag, that would be preferable too!

[marcrasi]

good idea, done

[marcrasi]

I've addressed all comments and uploaded a new commit.

I still want to add more tests, and I'll do that soon.

[marcrasi]

Actually, I think that testing and polishing this PR is going to take a lot more time and I would rather spend my last day before vacation doing some more immediately useful things. Therefore, I'm going to abandon this until I come back on Jan 2, 2020.

I will do one thing though: This PR requires some changes in upstreamed TypeCheckAttr.cpp code, so I'll send a master PR that makes those changes. Hopefully these changes will get downstreamed into tensorflow through the regular merge processes before I resume work on this PR :)

[dan-zheng]

Are @derivative attributes in the primary file already type-checked (e.g. parameter indices are resolved) after TypeChecker::typeCheckDerivativeAttrs is called in TypeCheckSourceFileRequest::evaluate?

If so, is AttributeChecker::visitDerivativeAttr doing redundant work (e.g. recomputing parameter indices)?

If so, can AttributeChecker::visitDerivativeAttr be changed to assert that attr is already type-checked, and to avoid redundant work by only checking for duplicate @derivative attributes?

[marcrasi]

Yes, there is redundant work happening. I can't deduplicate the work by doing exactly what you say because the second typechecking uses some of the intermediate calculations to make diagnostic messages (e.g. original function not found messages). So I need to either redo the computation or find some way of passing the intermediate calculations from the first time it typechecks to the second time it typechecks.

Here's an idea that could work:

static bool typeCheckDerivativeAttr(...) {
  if (!attr->getOriginalFunction() || !attr->getParameterIndices()) {
    // Do typechecking
    // If successful, set original function and parameter indices
    // If failure, diagnose and return true.
  }

  // Insert attr into Ctx.DerivativeAttrs
  // Diagnose duplicates
}

That way, if the first round of typechecking is successful, then we don't duplicate the work on the second round. If the first round fails, we duplicate the work during the second round, but speed is less important in the failure case so this isn't terrible.

[dan-zheng]

I can't deduplicate the work by doing exactly what you say because the second typechecking uses some of the intermediate calculations to make diagnostic messages (e.g. original function not found messages).

Aha, that makes sense.

Here's an idea that could work:

static bool typeCheckDerivativeAttr(...) {
  if (!attr->getOriginalFunction() || !attr->getParameterIndices()) {
    // Do typechecking
    // If successful, set original function and parameter indices
    // If failure, diagnose and return true.
  }

  // Insert attr into Ctx.DerivativeAttrs
  // Diagnose duplicates
}

That way, if the first round of typechecking is successful, then we don't duplicate the work on the second round. If the first round fails, we duplicate the work during the second round, but speed is less important in the failure case so this isn't terrible.

This idea sounds great! Duplicate work only for bad attributes seems very acceptable.

[marcrasi]

Closing because I'm going to do this as separate incremental pieces gated behind the flag (#28891) instead of one large PR.