Skip to content
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

Autodiff Upstreaming - enzyme backend #129176

Merged
merged 1 commit into from
Sep 6, 2024
Merged

Conversation

ZuseZ4
Copy link
Contributor

@ZuseZ4 ZuseZ4 commented Aug 17, 2024

Tracking issue: #124509

Part of #129175

This PR should allow building Enzyme from source on Tier 1 targets (when also building LLVM), except MSVC.
It's only a small fraction (~200 lines) of the whole upstream PR, but due to bootstrapping and the number of configurations in which rustc can be build I assume that this will be the hardest to merge, so I'm starting with it.
Happy to hear what changes are required to be able to upstream this code.

Content:
It contains a new configure flag --enable-llvm-enzyme, and will build the new Enzyme submodule when it is set.

Discussion:
Apparently Rust CI isn't able to clone repositories outside the rust-lang org? At least I'm seeing this error in CI:

[email protected]: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Does that mean we would need to mirror github.com/EnzymeAD/Enzyme in rust-lang, until LLVM upgrades Enzyme from an Incubator project to something that ships as part of the monorepo?

Tracking:

@rustbot
Copy link
Collaborator

rustbot commented Aug 17, 2024

r? @albertlarsan68

rustbot has assigned @albertlarsan68.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Aug 17, 2024

⚠️ Warning ⚠️

  • These commits modify submodules.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 17, 2024
@rust-log-analyzer

This comment has been minimized.

@ZuseZ4 ZuseZ4 changed the title enzyme backend Autodiff Upstreaming - enzyme backend Aug 17, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Aug 17, 2024

Use HTTPS instead of SSH authentication for the git repo, then it should work.

.gitmodules Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Kobzol
Copy link
Contributor

Kobzol commented Aug 17, 2024

You should probably add an ignore for the enzyme submodule in tidy.

@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Aug 17, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@ZuseZ4 ZuseZ4 marked this pull request as ready for review August 17, 2024 17:06
@rustbot
Copy link
Collaborator

rustbot commented Aug 17, 2024

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@ZuseZ4
Copy link
Contributor Author

ZuseZ4 commented Aug 17, 2024

Thank you two for already having looked at it. CI passes now, how would we continue?

We probably only want to configure the llvm_enzyme flag as enabled for nightly builds shipped to users once all the other code including tests are moved over to rustc. So from my side, I would be fine with squashing and merging this and putting up the next PR which would include our autodiff rustc_builtin_macro. The frontend is pretty lightweight to build, so we could already enable it by default and just error out in the middle-end, similar to how f128 did it (still do it?).

@jieyouxu jieyouxu added the F-autodiff `#![feature(autodiff)]` label Aug 18, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 18, 2024

This PR modifies config.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 18, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 18, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

@ZuseZ4
Copy link
Contributor Author

ZuseZ4 commented Aug 18, 2024

I guess I shouldn't use the GUI here to resolve merge conflicts, lesson learned.
I intend clean the history up once this PR got approved to not disturb anyone reviewing, but I can also do it now if anyone prefers that.
(I had picked 4 more lines (in src/bootstrap/configure.py and config.example.toml) from the full PR to this one and adjusted the change_tracker accordingly based on the bot feedback I got above, but this caused the merge conflict).

@rustbot rustbot removed the has-merge-commits PR has merge commits, merge with caution. label Aug 18, 2024
@bors
Copy link
Contributor

bors commented Sep 4, 2024

📌 Commit 14e8f45 has been approved by albertlarsan68

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 4, 2024
@traviscross
Copy link
Contributor

@bors rollup=never

Co-authored-by: Lorenz Schmidt <[email protected]>
@traviscross
Copy link
Contributor

@bors r=albertlarsan68 p=1

(This seems likely to attract conflicts. The latest push was due to such a conflict developing while in queue.)

@bors
Copy link
Contributor

bors commented Sep 6, 2024

📌 Commit 4f5c16d has been approved by albertlarsan68

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 6, 2024

⌛ Testing commit 4f5c16d with merge f6e192a...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 6, 2024
…san68

Autodiff Upstreaming - enzyme backend

Tracking issue: rust-lang#124509

Part of rust-lang#129175

This PR should allow building Enzyme from source on Tier 1 targets (when also building LLVM), except MSVC.
It's only a small fraction (~200 lines) of the whole upstream PR, but due to bootstrapping and the number of configurations in which rustc can be build I assume that this will be the hardest to merge, so I'm starting with it.
Happy to hear what changes are required to be able to upstream this code.

**Content:**
It contains a new configure flag `--enable-llvm-enzyme`, and will build the new Enzyme submodule when it is set.

**Discussion:**
Apparently Rust CI isn't able to clone repositories outside the rust-lang org? At least I'm seeing this error in CI:
```
[email protected]: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
```
Does that mean we would need to mirror github.com/EnzymeAD/Enzyme in rust-lang, until LLVM upgrades Enzyme from an Incubator project to something that ships as part of the monorepo?

Tracking:

- rust-lang#124509
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] miri test:false 4.457
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Fri, Sep  6, 2024  7:16:49 AM
  network time: Fri, 06 Sep 2024 07:16:49 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@bors
Copy link
Contributor

bors commented Sep 6, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 6, 2024
@traviscross
Copy link
Contributor

@bors retry

cc #127883

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2024
@bors
Copy link
Contributor

bors commented Sep 6, 2024

⌛ Testing commit 4f5c16d with merge 59d4114...

@bors
Copy link
Contributor

bors commented Sep 6, 2024

☀️ Test successful - checks-actions
Approved by: albertlarsan68
Pushing 59d4114 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 6, 2024
@bors bors merged commit 59d4114 into rust-lang:master Sep 6, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 6, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (59d4114): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.6% [-3.6%, -3.6%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 2.2%, secondary -2.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.2% [2.2%, 2.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.4% [-2.6%, -2.1%] 2
All ❌✅ (primary) 2.2% [2.2%, 2.2%] 1

Cycles

Results (secondary 2.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.7% [2.3%, 3.0%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 754.927s -> 756.647s (0.23%)
Artifact size: 340.95 MiB -> 340.92 MiB (-0.01%)

@ZuseZ4 ZuseZ4 deleted the enzyme-backend branch September 11, 2024 19:59
@jieyouxu jieyouxu added the CI-spurious-fail-msvc CI spurious failure: target env msvc label Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc CI-spurious-fail-msvc CI spurious failure: target env msvc F-autodiff `#![feature(autodiff)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.