Skip to content

[AutoDiff] Enhance performance of custom derivatives lookup #76951

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

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

kovdan01
Copy link
Contributor

@kovdan01 kovdan01 commented Oct 10, 2024

In #58965, lookup for custom derivatives in non-primary source files was
introduced. It required triggering delayed members parsing of nominal types in
a file if the file was compiled with differential programming enabled.

This patch introduces CustomDerivativesRequest to address the issue.
We only parse delayed members if tokens @ and derivative appear
together inside skipped nominal type body (similar to how member operators
are handled).

Resolves #60102

@kovdan01
Copy link
Contributor Author

Tagging @asl

Verified

This commit was signed with the committer’s verified signature.
topaxi Damian Senn
In swiftlang#58965, lookup for custom derivatives in non-primary source files was
introduced. It required triggering delayed members parsing of nominal types in
a file if the file was compiled with differential programming enabled.

This patch introduces `CustomDerivativesRequest` to address the issue.
We only parse delayed members if tokens `@` and `derivative` appear
together inside skipped nominal type body (similar to how member operators
are handled).

Resolves swiftlang#60102
@asl
Copy link
Contributor

asl commented Oct 29, 2024

@swift-ci please test

@asl
Copy link
Contributor

asl commented Oct 29, 2024

@slavapestov This implements your request from #58965 (comment)

@kovdan01
Copy link
Contributor Author

kovdan01 commented Nov 5, 2024

Would be glad to see feedback from everyone interested

@kovdan01
Copy link
Contributor Author

kovdan01 commented Nov 6, 2024

Also tagging @rxwei

@kovdan01
Copy link
Contributor Author

Would be glad to see feedback from everyone interested

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I have one non-actionable comment, but let's go ahead and merge

Comment on lines +286 to +294
if (onlyDerivatives)
if (AbstractFunctionDecl *AFD = getDerivative())
CustomDerivatives.push_back(AFD);
if (!onlyOperators && !onlyDerivatives && VD->hasName()) {
TopLevelValues.add(VD);
if (VD->getAttrs().hasAttribute<CustomAttr>())
MayHaveAuxiliaryDecls.push_back(VD);
}
if (AbstractFunctionDecl *AFD = getDerivative())
CustomDerivatives.push_back(AFD);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a specific suggestion on how to improve it, but this logic was a little tangled before with onlyOperators and now it's... really hard to sort through.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bringing attention to this! I agree that the logic is pretty complicated and should be simplified somehow. I was also unable to quickly find a nice way to fix this, but I'll think of it and submit a subsequent PR once the refactoring of this function is ready.

@DougGregor
Copy link
Member

@swift-ci please smoke test

@DougGregor
Copy link
Member

Kicking off CI once more just to be sure, then once it's green let's merge

@DougGregor DougGregor merged commit cf68d28 into swiftlang:main Nov 11, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants