Skip to content

Autodiff Upstreaming - enzyme backend - #129176

Merged
bors merged 1 commit into
rust-lang:masterfrom
EnzymeAD:enzyme-backend
Sep 6, 2024
Merged

bors merged 1 commit into
rust-lang:masterfrom
EnzymeAD:enzyme-backend

Conversation

@ZuseZ4

@ZuseZ4 ZuseZ4 commented Aug 17, 2024 •

Copy link
Copy Markdown
Member

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:

git@github.com: 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

rustbot commented Aug 17, 2024

Copy link
Copy Markdown
Collaborator

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

rustbot commented Aug 17, 2024

Copy link
Copy Markdown
Collaborator

⚠️ 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

Kobzol commented Aug 17, 2024 •

Copy link
Copy Markdown
Member

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

Comment thread .gitmodules Outdated
@rust-log-analyzer

This comment has been minimized.

Comment thread src/bootstrap/src/core/build_steps/compile.rs Outdated
@rust-log-analyzer

This comment has been minimized.

@Kobzol

Kobzol commented Aug 17, 2024

Copy link
Copy Markdown
Member

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

rustbot commented Aug 17, 2024

Copy link
Copy Markdown
Collaborator

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

ZuseZ4 commented Aug 17, 2024 •

Copy link
Copy Markdown
Member Author

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?).

Comment thread src/bootstrap/src/core/build_steps/llvm.rs Outdated
@jieyouxu jieyouxu added the F-autodiff `#![feature(autodiff)]` label Aug 18, 2024
@rustbot

rustbot commented Aug 18, 2024

Copy link
Copy Markdown
Collaborator

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

rustbot commented Aug 18, 2024

Copy link
Copy Markdown
Collaborator

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

ZuseZ4 commented Aug 18, 2024 •

Copy link
Copy Markdown
Member Author

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

bors commented Sep 4, 2024

Copy link
Copy Markdown
Collaborator

📌 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
Copy Markdown
Contributor

@bors rollup=never

Co-authored-by: Lorenz Schmidt <bytesnake@mailbox.org>
@traviscross

Copy link
Copy Markdown
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

bors commented Sep 6, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit 4f5c16d has been approved by albertlarsan68

It is now in the queue for this repository.

@bors

bors commented Sep 6, 2024

Copy link
Copy Markdown
Collaborator

⌛ 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:
```
git@github.com: 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
Copy Markdown
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

bors commented Sep 6, 2024

Copy link
Copy Markdown
Collaborator

💔 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
Copy Markdown
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

bors commented Sep 6, 2024

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Sep 6, 2024

Copy link
Copy Markdown
Collaborator

☀️ 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
@rustbot rustbot added this to the 1.83.0 milestone Sep 6, 2024
@rust-timer

Copy link
Copy Markdown
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.