Skip to content

Support differentiation of wrapped value modify accessors #78794

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 10, 2025
Merged

Support differentiation of wrapped value modify accessors #78794

merged 4 commits into from
Sep 10, 2025

Conversation

asl
Copy link
Contributor

@asl asl commented Jan 21, 2025

Some fixes for coroutines with normal results and partial_apply of coroutines were required.

Fixes #55084

@asl asl requested a review from rxwei January 21, 2025 20:49
@asl
Copy link
Contributor Author

asl commented Jan 21, 2025

@swift-ci please test

@asl
Copy link
Contributor Author

asl commented Jan 21, 2025

Tagging @JaapWijnen

@asl asl added the AutoDiff label Jan 21, 2025
@asl
Copy link
Contributor Author

asl commented Jan 21, 2025 •
edited
Loading

The addition of derivative to Tracked<T>.*= is trivial: the operator is marked as @differentiable but no custom derivative was specified. It does not influence ABI as it is a part of DifferentiableUnittests module

@asl
Copy link
Contributor Author

asl commented Jan 24, 2025

@swift-ci please test linux

@asl asl force-pushed the diff-modify-property-wrappers branch from c675009 to b4a8fc0 Compare April 2, 2025 06:18
@asl
Copy link
Contributor Author

asl commented Apr 2, 2025

@swift-ci please test

@asl
Copy link
Contributor Author

asl commented Apr 7, 2025

ping

@JaapWijnen
Copy link
Contributor

This is a prerequisite to the PR enabling custom derivatives from _modify accessors right @asl ? (#78508)

@asl
Copy link
Contributor Author

asl commented May 12, 2025

This is a prerequisite to the PR enabling custom derivatives from _modify accessors right @asl ? (#78508)

It contains common codegen fixes. So I would like to have them committed first and separately to reduce amount of changes in #78508

@asl
Copy link
Contributor Author

asl commented May 19, 2025 •
edited
Loading

@swiftlang/standard-librarians Are you ok with stdlib changes? They are internal as only add couple of missed derivatives to stdlib/private/DifferentiationUnittest/DifferentiationUnittest.swift.gyb

@asl
Copy link
Contributor Author

asl commented May 19, 2025

@rjmccall Will you please review irgen changes? They are essentially ensures that coroutine return value is lowered in the proper generic context. See 7b2fecc

@asl asl force-pushed the diff-modify-property-wrappers branch from b4a8fc0 to 63ab094 Compare May 19, 2025 11:28
@asl
Copy link
Contributor Author

asl commented May 19, 2025

@swift-ci please test

@asl asl force-pushed the diff-modify-property-wrappers branch from 63ab094 to 6fac043 Compare July 11, 2025 16:33
@asl asl removed the request for review from a team July 11, 2025 16:33
@asl
Copy link
Contributor Author

asl commented Jul 11, 2025

@swift-ci please test

@asl
Copy link
Contributor Author

asl commented Jul 11, 2025

@rjmccall another friendly ping

@asl
Copy link
Contributor Author

asl commented Jul 31, 2025

@rjmccall @eeckstein ping

asl
asl commented Jul 31, 2025
View reviewed changes
lib/IRGen/IRGenSIL.cpp

if (!isAbort) {
auto resultType = call->getType();
Explosion e;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was likely some mis-merge from the previous PRs. Unfortunately, it requires proper non-trivial coroutine result to see the difference. Now this codepath is executed via tests.

@asl
Copy link
Contributor Author

asl commented Jul 31, 2025

@rjmccall @eeckstein Will you please review IRGen changes? They are necessary to ensure that coroutine result is lowered in an appropriate (caller) context. In addition there is one mismerge fix that is now covered by the tests.

@eeckstein
Copy link
Contributor

eeckstein commented Jul 31, 2025 •
edited
Loading

Will you please review IRGen changes?

@aschwaighofer would be a better person than me to review this

@asl
Copy link
Contributor Author

asl commented Jul 31, 2025

@aschwaighofer Will you please take a look?

@asl
Copy link
Contributor Author

asl commented Aug 18, 2025

@aschwaighofer Will you please take a look on irgen changes? There are comments there explaining them.

Thanks!

@asl asl requested a review from aschwaighofer August 18, 2025 16:35
@asl
Copy link
Contributor Author

asl commented Sep 2, 2025

@aschwaighofer @rjmccall ping

@asl
Copy link
Contributor Author

asl commented Sep 5, 2025

Any smallest chance to get irgen fixes reviewed? @rjmccall @aschwaighofer @tkremenek @DougGregor ?

@tkremenek
Copy link
Member

Any smallest chance to get irgen fixes reviewed? @rjmccall @aschwaighofer @tkremenek @DougGregor ?

I’m looking to see if we can get a review soon.

@asl
Copy link
Contributor Author

asl commented Sep 9, 2025

Any smallest chance to get irgen fixes reviewed? @rjmccall @aschwaighofer @tkremenek @DougGregor ?

I’m looking to see if we can get a review soon.

Thanks a lot, much appreciated!

@JaapWijnen
Copy link
Contributor

Thanks! Really appreciate it!

aschwaighofer
aschwaighofer approved these changes Sep 9, 2025
View reviewed changes
Copy link
Contributor

@aschwaighofer aschwaighofer left a comment

Choose a reason for hiding this comment

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

The IRGen changes look good to me.

@asl asl force-pushed the diff-modify-property-wrappers branch from 6fac043 to b5cceba Compare September 9, 2025 23:18
@asl
Copy link
Contributor Author

asl commented Sep 9, 2025

The IRGen changes look good to me.

Thanks!

@asl
Copy link
Contributor Author

asl commented Sep 9, 2025

@swift-ci please smoke test

@asl
Copy link
Contributor Author

asl commented Sep 10, 2025

@swift-ci please smoke test macos

@asl asl enabled auto-merge (squash) September 10, 2025 07:05
@asl asl merged commit 7668666 into main Sep 10, 2025
3 checks passed
@asl asl deleted the diff-modify-property-wrappers branch September 10, 2025 09:30
@JaapWijnen JaapWijnen mentioned this pull request Sep 10, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aschwaighofer aschwaighofer aschwaighofer approved these changes

@eeckstein eeckstein Awaiting requested review from eeckstein eeckstein is a code owner

@rjmccall rjmccall Awaiting requested review from rjmccall rjmccall is a code owner

@hborla hborla Awaiting requested review from hborla

@slavapestov slavapestov Awaiting requested review from slavapestov

@xedin xedin Awaiting requested review from xedin

@rxwei rxwei Awaiting requested review from rxwei

+1 more reviewer

@JaapWijnen JaapWijnen JaapWijnen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AutoDiff

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[SR-12640] Differentiation transform: support wrapped value modify accessors

5 participants

@asl @JaapWijnen @eeckstein @tkremenek @aschwaighofer