-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Autodiff flags #139700
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?
Autodiff flags #139700
Conversation
4b475ba
to
0c157e9
Compare
This comment has been minimized.
This comment has been minimized.
@@ -576,12 +577,14 @@ pub(crate) unsafe fn llvm_optimize( | |||
// optimizations until after differentiation. Our pipeline is thus: (opt + enzyme), (full opt). | |||
// We therefore have two calls to llvm_optimize, if autodiff is used. | |||
if consider_ad && autodiff_stage != AutodiffStage::PostAD { | |||
merge_functions = false; |
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.
is there a way to do this on a per-function basis with some llvm attribute?
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.
Based on https://github.com/llvm/llvm-project/blob/9483aaaaaa427b5dcb9a7af8f232a4696eef94bf/llvm/lib/Transforms/IPO/MergeFunctions.cpp#L430 either setting their linkage (hasAvailableExternallyLinkage) or hasDistinctMetadataIntrinsic would do the trick. I'll try to quickly hack something together.
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.
Ok, after some experimenting I think it's not worth the cost here. Setting hasExternalLinkage will cause our body to be dropped (since it should be linked externally), so we drop/delete the function which we want to differentiate. That's a very effective way of preventing merging, but kind of problematic if we want to differentiate it. I could insert a random intrinsic and generate some arguments for it and add the Distinct metadata to one of the arguments, but I think the body of our dummy functions is already messy enough and I'd rather spend time towards moving to the declaration, which will sidestep this problem.
Interestingly, it seems that some other projects have conflicts with exactly the same LLVM optimization passes as autodiff.
At least
LLVMRustOptimize
has exactly the flags that we need to disable problematic opt passes.This PR enables us to compile code where users differentiate two identical functions in the same module. This has been especially common in test cases, but it's not impossible to encounter in the wild.
It also enables two new flags for testing/debugging. I consider writing an MCP to upgrade PrintPasses to be a standalone -Z flag, since it is not the same as
-Z print-llvm-passes
, which IMHO gives less useful output. A discussion can be found here: #t-compiler/llvm > Print llvm passes. @ 💬Finally, it improves
PrintModBefore
andPrintModAfter
. They used to work reliable, but now we just schedule enzyme as part of an existing ModulePassManager (MPM). Since Enzyme is last in the MPM scheduling, PrintModBefore became very inaccurate. It used to print the input module, which we gave to the Enzyme and was great to create llvm-ir reproducer. However, lately the MPM would run the wholedefault<O3>
pipeline, which heavily modifies the llvm module, before we pass it to Enzyme. That made it impossible to use the flag to create llvm-ir reproducers for Enzyme bugs. We now schedule a PrintModule pass just before Enzyme, solving this problem.Based on the PrintPass output, it also seems like changing
registerEnzymeAndPassPipeline(PB, true);
toregisterEnzymeAndPassPipeline(PB, false);
has no effect. In theory, the bool should tell Enzyme to schedule some helpful passes in the PassBuilder. However, since it doesn't do anything and I'm not 100% sure anymore on whether we really need it, I'll just disable it for now and postpone investigations.r? @oli-obk
closes #139471
Tracking: