Skip to content

adding UI 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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

KMJ-007
Copy link
Contributor

@KMJ-007 KMJ-007 commented Jun 13, 2025

r? @ZuseZ4

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 13, 2025
@ZuseZ4
Copy link
Member

ZuseZ4 commented Jun 13, 2025

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.
Instead, you should pass it just like all the other flags I'm passing to Enzyme, you can find the handling in rustc_codegen_llvm/src/back/write/lto.rs. First try to pass a hard-coded function name to enzyme, and make it print TA just for that function. We can later generalize it.

I'd focus for the next days on first getting the flags tested and supported, then you can continue with the tests here.

@ZuseZ4 ZuseZ4 added the F-autodiff `#![feature(autodiff)]` label Jun 27, 2025
@rust-cloud-vms rust-cloud-vms bot force-pushed the autodiff-codegen-test branch from 957d82f to 402f34c Compare July 1, 2025 03:47
@rustbot rustbot added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Jul 1, 2025
@rust-log-analyzer

This comment has been minimized.

return LLVMRustResult::Failure;
}
}

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 changes are for fixing PrintTAFn flag, which was not working, should i create new PR for this?

Copy link
Member

Choose a reason for hiding this comment

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

No need for it, just make it it's own commit and that the history is clean.
E.g. end up with a first commit fixing the Wrapper, and a second commit introducing the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I will clean the history after I am done writing tests at the end

@rust-log-analyzer

This comment has been minimized.

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

@ZuseZ4 ZuseZ4 Jul 1, 2025

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.

Copy link
Member

@jieyouxu jieyouxu Jul 1, 2025

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

Copy link
Contributor Author

@KMJ-007 KMJ-007 Jul 1, 2025

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"

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

@ZuseZ4 ZuseZ4 Jul 1, 2025

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

Copy link
Member

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 {

Copy link
Member

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.

@KMJ-007 KMJ-007 changed the title adding codegen test to autodiff adding UI test to autodiff Jul 1, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@KMJ-007 KMJ-007 requested a review from ZuseZ4 July 1, 2025 10:37
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2025
@KMJ-007 KMJ-007 marked this pull request as ready for review July 1, 2025 10:37
@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2025

Some changes occurred in src/tools/enzyme

cc @ZuseZ4

@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2025

⚠️ Warning ⚠️

  • Some commits in this PR modify submodules.

  • The following commits have 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
    

@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 Jul 1, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-cloud-vms rust-cloud-vms bot force-pushed the autodiff-codegen-test branch from 279a3a3 to 3c049de Compare July 1, 2025 19:33
@rust-log-analyzer

This comment has been minimized.

@rust-cloud-vms rust-cloud-vms bot force-pushed the autodiff-codegen-test branch from 3c049de to 8e1cdb3 Compare July 1, 2025 19:59
@KMJ-007 KMJ-007 requested a review from ZuseZ4 July 1, 2025 20:00
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 1, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-cloud-vms rust-cloud-vms bot force-pushed the autodiff-codegen-test branch from 8e1cdb3 to 49de074 Compare July 2, 2025 02:58
@rust-log-analyzer

This comment has been minimized.

@rust-cloud-vms rust-cloud-vms bot force-pushed the autodiff-codegen-test branch from 49de074 to 49510d0 Compare July 2, 2025 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. F-autodiff `#![feature(autodiff)]` has-merge-commits PR has merge commits, merge with caution. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants