-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[AutoDiff] Implement cross-file lookup of derivatives #58644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
#include "swift/AST/ASTContext.h" | ||
#include "swift/AST/ASTWalker.h" | ||
#include "swift/AST/ASTMangler.h" | ||
#include "swift/AST/Attr.h" | ||
#include "swift/AST/CaptureInfo.h" | ||
#include "swift/AST/DiagnosticEngine.h" | ||
#include "swift/AST/DiagnosticsSema.h" | ||
|
@@ -8310,7 +8311,7 @@ void AbstractFunctionDecl::prepareDerivativeFunctionConfigurations() { | |
} | ||
|
||
ArrayRef<AutoDiffConfig> | ||
AbstractFunctionDecl::getDerivativeFunctionConfigurations() { | ||
AbstractFunctionDecl::getDerivativeFunctionConfigurations(bool lookInNonPrimarySources) { | ||
prepareDerivativeFunctionConfigurations(); | ||
|
||
// Resolve derivative function configurations from `@differentiable` | ||
|
@@ -8333,6 +8334,37 @@ AbstractFunctionDecl::getDerivativeFunctionConfigurations() { | |
ctx.loadDerivativeFunctionConfigurations(this, previousGeneration, | ||
*DerivativeFunctionConfigs); | ||
} | ||
|
||
class DerivativeFinder : public ASTWalker { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drive-by question: does When a file is updated, it and all files that depend on it need to be recompiled. Incremental compilation is fast when there are few dependencies between files. I wonder if
Initially,
Since all files ( Is the † recompilation behavior asymptotically worse than existing incremental compilation? It might not be, if the same amount of work (e.g. recompiling the entire module) was necessary previous to the implementation of correct There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (It may be that the impact of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dan-zheng The complete recompilation is certainly necessary when optimizations are enabled. As derivatives might be inlined and subsequently optimized. So, effectively we'd need to throw out all the code and start afresh. I do not like the existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When optimizations are enabled you're building in whole-module mode anyways. Swift will refuse to perform the incremental build if you ask for -wmo as well. An even simpler failure mode occurs here since this analysis is not going through name lookup. From the incremental build's perspective, there is no link between the derivative and the original. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm confused. Will you please elaborate? The original testcase from #55170 fails regardless whether |
||
const AbstractFunctionDecl *AFD; | ||
public: | ||
DerivativeFinder(const AbstractFunctionDecl *afd) : AFD(afd) {} | ||
|
||
bool walkToDeclPre(Decl *D) override { | ||
if (auto *afd = dyn_cast<AbstractFunctionDecl>(D)) { | ||
for (auto *derAttr : afd->getAttrs().getAttributes<DerivativeAttr>()) { | ||
// Resolve derivative function configurations from `@derivative` | ||
// attributes by type-checking them. | ||
if (AFD->getName().matchesRef( | ||
derAttr->getOriginalFunctionName().Name.getFullName())) { | ||
(void)derAttr->getOriginalFunction(afd->getASTContext()); | ||
return false; | ||
} | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
}; | ||
|
||
// Load derivative configurations from @derivative attributes defined in | ||
// non-primary sources. Note that it might trigger lookup cycles if called | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This analysis needs to go through name lookup. If that's triggering cycles then we need to figure out why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It cannot by design. See the full description in #58644 (comment) There is no way you could discover custom derivatives via name lookup as name is not known and the original function does not know anything about custom derivatives. |
||
// from inside Sema stages. | ||
if (lookInNonPrimarySources) { | ||
DerivativeFinder finder(this); | ||
getParent()->walkContext(finder); | ||
} | ||
|
||
return DerivativeFunctionConfigs->getArrayRef(); | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.