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

adding rust ADBench version #1797

Closed
wants to merge 90 commits into from
Closed

adding rust ADBench version #1797

wants to merge 90 commits into from

Conversation

ZuseZ4
Copy link
Member

@ZuseZ4 ZuseZ4 commented Mar 19, 2024

Usage for Rust side

cargo +enzyme run --release
Comipling a lib.so to use from C:
cargo +enzyme rustc --release --lib --crate-type=cdylib

@ZuseZ4 ZuseZ4 requested a review from wsmoses March 19, 2024 04:13
@ZuseZ4
Copy link
Member Author

ZuseZ4 commented Mar 19, 2024

Usage instruction: cargo +enzyme build
fails with

  todo - hook in rust allocation fns
  UNREACHABLE executed at /h/344/drehwald/prog/rust/src/tools/enzyme/enzyme/Enzyme/GradientUtils.cpp:9107!

llvm_unreachable("todo - hook in rust allocation fns");

@ZuseZ4
Copy link
Member Author

ZuseZ4 commented Mar 19, 2024

printing the module before AD (with a few helping infos at the end)
err.ll.txt

@ZuseZ4 ZuseZ4 changed the title adding gmm adding rust ADBench version Apr 3, 2024
@@ -127,6 +127,19 @@ extern "C" {
double* reproj_err,
double* w_err
);

void rust2_ba_objective(
Copy link
Member

Choose a reason for hiding this comment

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

weak symbol variant of this.

-> running these doesn't require rust.

%-opt.ll: %-raw.ll
opt $^ -o $@ -S
#opt $^ -O2 -o $@ -S
$(dir)/benchmarks/ReverseMode/ba/target/release/libbars.a: src/lib.rs Cargo.toml
Copy link
Member

Choose a reason for hiding this comment

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

if this can be optional?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What would you like this variable to use when determining if Rust is enabled? We could run cargo +enzyme version (checking whether a toolchain named enzyme is configured) and/or we could have an environment variable to be set (by CMake or otherwise).

Comment on lines 34 to 63
fn radix2(data: &mut [f64], i_sign: f64) {
let n = data.len() / 2;
if n == 1 {
return;
}

let (a,b) = data.split_at_mut(n);
radix2(a, i_sign, n/2);
radix2(b, i_sign, n/2);
let (a, b) = data.split_at_mut(n);
radix2(a, i_sign);
radix2(b, i_sign);

let wtemp = i_sign * (PI / n as f64).sin();
let wpi = -i_sign * (2.0 * PI / n as f64).sin();
let wpr = -2.0 * wtemp * wtemp;
let mut wr = 1.0;
let mut wi = 0.0;

assert_eq!(data.len(), 2*n);
for i in (0..n).step_by(2) {
let in_n = i + n;
let tempr = data[in_n] * wr - data[in_n + 1] * wi;
let tempi = data[in_n] * wi + data[in_n + 1] * wr;
for i in (0..n).step_by(2) {
let tempr = b[i] * wr - b[i + 1] * wi;
let tempi = b[i] * wi + b[i + 1] * wr;

data[in_n] = data[i] - tempr;
data[in_n + 1] = data[i + 1] - tempi;
data[i] += tempr;
data[i + 1] += tempi;
b[i] = a[i] - tempr;
b[i + 1] = a[i + 1] - tempi;
a[i] += tempr;
a[i + 1] += tempi;

let wtemp_new = wr;
wr += wr * wpr - wi * wpi;
wi += wi * wpr + wtemp_new * wpi;
}

}
Copy link
Collaborator

@jedbrown jedbrown Oct 31, 2024

Choose a reason for hiding this comment

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

@ZuseZ4 Let me know if you don't like this and I'll remove it. (Other changes in this commit are just rustfmt/whitespace.)

jedbrown and others added 21 commits October 31, 2024 14:11
* main: (49 commits)
  Fix iv of constant (#2141)
  Update benchmarks (#2035)
  Implement tgamma derivative (#2140)
  tgamma error improvement (#2139)
  Improve cache index error message (#2138)
  Fixes warnings and adds missing header guards (#2124)
  mlir: cache and reuse reverse funcs (#2133)
  mlir: implement forward mode for func.call (#2134)
  mlir: Func call reverse diff (#2127)
  Update build_tarballs.jl
  Fix combined temp cache for reverse (#2131)
  Improve runtime activity err message (#2132)
  Fix undef value storage (#2129)
  Adapt to const tblgen (#2128)
  Add gcloaded TT (#2125)
  Fix blas decl updater indexing (#2123)
  Add header files to ClangEnzyme target (#2062)
  Improve unknown function error messages (#2120)
  Fix handle sync (#2122)
  Support more Julia 1.11 functions (#2121)
  ...
This yields identical assembly apart from asserts at the top of safe::radix2.
Note that -mtune=cascadelake is only about tuning (like instruction
timing) not arch/features.
@ZuseZ4
Copy link
Member Author

ZuseZ4 commented Nov 26, 2024

closing as discussed, since maintaining this benchmark infra turned out to be too much work and this has no outlook of being merged.
I'll maintain the Rust implementation elsewhere, to avoid regressions.

@ZuseZ4 ZuseZ4 closed this Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants