-
Notifications
You must be signed in to change notification settings - Fork 13.5k
adding run-make test to autodiff #142444
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
base: master
Are you sure you want to change the base?
adding run-make test to autodiff #142444
Conversation
You'll need the PrintTA flag, look in the rustc-dev-guide for the exact spelling. That should at least give you some typetree metadata. You likely also want to first merge support for the more specific flag we discussed where you can pass a specific function name. Look into enzyme to find out the name of the flag and grep for how enzyme handles it internally. We likely want to pass that flag to enzyme not via the command line, since we call libEnzyme, which doesn't accept command line flags. I'd focus for the next days on first getting the flags tested and supported, then you can continue with the tests here. |
957d82f
to
402f34c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
tests/ui/autodiff/f64.stderr
Outdated
updating analysis of val: %2 = load double, ptr %0, align 8, !dbg !384, !noundef !14 current: {} new {[-1]:Float@double} from %3 = fmul double %2, %2, !dbg !385 Changed=1 legal=1 | ||
updating analysis of val: %2 = load double, ptr %0, align 8, !dbg !384, !noundef !14 current: {[-1]:Float@double} new {[-1]:Float@double} from %3 = fmul double %2, %2, !dbg !385 Changed=0 legal=1 | ||
updating analysis of val: %3 = fmul double %2, %2, !dbg !385 current: {[-1]:Float@double} new {[-1]:Float@double} from %3 = fmul double %2, %2, !dbg !385 Changed=0 legal=1 | ||
updating analysis of val: ptr %0 current: {[-1]:Pointer} new {[-1]:Pointer, [-1,0]:Float@double} from %2 = load double, ptr %0, align 8, !dbg !384, !noundef !14 Changed=1 legal=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd assume that the dbg/noundef numbers like 384, 385 and 14 will change soon enough. Best case you replace all fixed constants like 384 and 385 with different variables, there's probably a good regex for it. I.e. on the first use of 384 you assign a name to it, and on all following usages of 384 you assure that it's the same name (value) showing up. There should be examples in Enzyme and the LLVM lit docs on how to do that.
Otherwise at least make sure that all of these two numbers are replaced with an anonymous placeholder, so if they change tests don't break.
Edit: now that we have a UI test ignore the comment about LLVM's lit test, still check if there's a way to avoid test failures if the numbers change. cc @jieyouxu who might have a suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how you would meaningfully normalize these. Maybe instead, use FileCheck inside a run-make test?
Otherwise, this is going to break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am thinking of using this regex:
//@ normalize-stderr: "!(dbg|noundef) ![0-9]+" -> "!$1 !N"
//@ normalize-stderr: "%[0-9]+" -> "%X"
//@ normalize-stdout: "!(dbg|noundef) ![0-9]+" -> "!$1 !N"
//@ normalize-stdout: "%[0-9]+" -> "%X"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That normalization can still be fragile, because can't the instruction ordering also change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, any time there are multiple subsequent loads, they're independent enough that, while LLVM usually generates them in a reasonably reliable order, we do change things enough in our codebase that sometimes we change those orderings around. So we would want CHECK-DAG
for those lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@workingjubilee @jieyouxu these are currently ui tests since we test stderr/stdout output. Should they be something else? I don't think we can use the LLVM LIT test infrastructure in ui, or do we have a Rust equivalent which makes CHECK-DAG work?
In general, I want to make sure that we see all the type analysis info which enzyme deduces written out, even if it's looking fragile for now. His next PRs will start lowering Rust information to llvm metadata, which should be more reliable if we do it properly. In that case Enzyme will need to deduce less types, and we can narrow down what we're actually testing for (which is the metadata generated by us).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have "run arbitrary code to execute the test" tests, called "run-make" because they used to be makefiles, but they actually are Rust executables now: https://github.com/rust-lang/rust/blob/master/tests/run-make/README.md
You can use that to run filecheck:
pub fn llvm_filecheck() -> LlvmFilecheck { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think using FileCheck within a run-make test is going to be least fragile, because you can also "don't care" about attributes and whatever and only match on stuff you do care about.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Some changes occurred in src/tools/enzyme cc @ZuseZ4 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
279a3a3
to
3c049de
Compare
This comment has been minimized.
This comment has been minimized.
3c049de
to
8e1cdb3
Compare
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Karan Janthe <[email protected]>
I have migrated tests to run-make test, can you please review it again |
This comment has been minimized.
This comment has been minimized.
22552d5
to
972fb63
Compare
This comment has been minimized.
This comment has been minimized.
972fb63
to
111b54d
Compare
This comment has been minimized.
This comment has been minimized.
CI is failing for UI related tests, but after migration, there is no change in UI tests folder, can someon rerun the CI |
You can force PR CI to re-run by closing and reopening the PR. But I think the latest failure is genuine? |
This comment has been minimized.
This comment has been minimized.
how do i enable experimental feature in run-make tests? |
This comment has been minimized.
This comment has been minimized.
cf34cc8
to
d330d1b
Compare
@jieyouxu all green! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some superficial test infra side review feedback, I've not looked at actual test "content".
.input("array.rs") | ||
.arg("-Zautodiff=Enable,PrintTAFn=callee") | ||
.arg("-Zautodiff=NoPostopt") | ||
.arg("-Copt-level=3") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this can use opt_level("3")
. See https://doc.rust-lang.org/nightly/nightly-rustc/run_make_support/external_deps/rustc/struct.Rustc.html#method.opt_level for available helpers on rustc()
.
|
||
fn main() { | ||
// Compile the Rust file with the required flags, capturing both stdout and stderr | ||
let output = rustc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: by default, run-make
test's rustc()
will try to cross-compile for targets tested under CI, which includes some Tier 2 targets that do not support std. You may wish to limit this further if you only wish to check this as a host test, e.g. //@ ignore-cross-compile
.
I've not looked at the exact tests, but you may also wish to restrict which platforms these run-make tests run against. For instance, apple and windows Tier 1 targets? Or i686 msvc, etc.
// Write the outputs to files | ||
fs::write("array.stdout", stdout).unwrap(); | ||
fs::write("array.stderr", stderr).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: run_make_support
has an fs helper module re-export as run_make_support::rfs
(https://doc.rust-lang.org/nightly/nightly-rustc/run_make_support/rfs/index.html), which will produce a panic incl. the path, so this can be rfs::write("array.stdout", stdout);
(https://doc.rust-lang.org/nightly/nightly-rustc/run_make_support/rfs/fn.write.html).
Ditto on other test recipes.
.arg("-Zautodiff=NoPostopt") | ||
.arg("-Copt-level=3") | ||
.arg("-Clto=fat") | ||
.arg("-g") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: is the exact level of debuginfo important for these tests? If it is, consider explicitly spelling it out as debuginfo_level("2")
with a comment, ignore this review comment otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enzyme has a parser for debug info metadata. Enzyme should "translate" the debug info into "Enzyme Typetrees", which it will then use later for computing derivatives. In reality Enzyme's debug metadata parser was untested for too long and doesn't seem to do anything (based on the output of these tests). Still, I would hard-code the debug info level since it (in theory) could affect the test output.
Also, these tests are meant to capture the status-quo. In the second half Karan will generate TypeTrees directly from MIR, so it's good to have a before/after comparison with the test cases.
lgtm with the comments from jieyouxe addressed. Enzyme had a bit of typetree logic related to Rust, but wasn't running it in their CI, so (as demonstrated by this PR) both the code and the tests there broke. https://github.com/EnzymeAD/Enzyme/tree/main/enzyme/test/TypeAnalysis The exact test output doesn't matter too much for now, it looks vaguely sensible (although a bit inefficient, but that's expected). After his next PR Kiran will generate better typetrees directly from MIR, so we can do a nice before/after comparison based on the test output to verify what improved. |
d330d1b
to
a776076
Compare
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Karan Janthe <[email protected]>
a776076
to
80224c3
Compare
r? @ZuseZ4 |
Requested reviewer is already assigned to this pull request. Please choose another assignee. |
r? @ZuseZ4