Allow setting strict-alias defaults at config time#32
Conversation
|
As i mentioned on that issue (EnzymeAD#2885) this will result in additional confusion. @nikic LLVM options aren't usually configurable, right? My motivation for rejecting the PR in upstream is that if you have two builds of enzyme, it is likely to cause greater confusion when the default is different (including in our compiler explorer). Issues are not filed to this repo, but to upstream enzyme for which a different default is going to cause more user problems imo |
|
cc @workingjubilee [not sure what other llvm folks would have thoughts on, but please add them]. And @ZuseZ4 I'm happy to discuss in the upstream issue, but so far in the thirty minutes since you opened (and since closed) the issue, I am currently under the impression this will cause more problems than it solves (happy to be explained why otherwise) |
|
Well in that case it just sounds like Enzyme should default to strict-aliasing off, right? After all, that's the only correct default choice. I didn't believe that you want to make the enzyme default slower in favour of correctness, but obviously happy to change it there. |
|
That's not necsesarily the correct default, it's a question of what is the best choice for the majority of users. Happy to discuss at an enzyme meeting and or poll folks. |
|
Rust already has many divergent settings from upstream LLVM's default configuration, which is fairly close to the one that is shipped in practice by many distros (but those also customize many details), in favor of things that make sense for a software toolchain that cares about the things that Rust does. When we ship a component we do so with the intention that it be used with our other components, without regard to its general reuse, and we have historically had less support for using external components with different configurations than the one we ship when we have supported the use of such an external component. If people are not filing issues to rust-lang/rust first in the vast majority of cases then I would be very surprised. People have filed bugs against us for link.exe, which is very much not something we ship, but something we rely on for the platform. We usually have considered it appropriate to do the first pass of triage for bugs and only direct people upstream when their bug is clearly not addressable directly by us. At that point we usually explain what the issue is that they should file if we do not do so ourselves. |
|
...The divergence has been reduced over time, I should note, so "many" may be a bit much, on reflection. That, however, has mostly been a function of more of the patches that Rust needs going upstream sooner and us being able to hew more closely to LLVM's releases. There was a time when we could not really get a patch upstreamed in an expedient fashion for a variety of reasons. That's a fair bit in the rear-view window now, but at that time we did hold patches until we could get them up. But even now our build configuration is different and selects safer defaults for the way that LLVM will handle things like erroneous states, and for a while our LLVM configuration was incapable of doing things you would expect LLVM to be capable of doing (like compiling clang). |
|
sure, and rust should completely set its own relevant defaults (and does so for this flag here, within rust itself: https://github.com/rust-lang/rust/blob/f7da3c0d4b3a4cc291f8c800cc61549d27d14c49/compiler/rustc_codegen_llvm/src/llvm/enzyme_ffi.rs#L338). A flag does exist for the behavior rust wants, and this is set by default in rust. This PR changes the default behavior outside of rust, in the llvm opt tool, to instead behave differently than upstream / default. The benefit is that users using a rust-built enzyme+llvm don't need to remember to pass the rust flag. The downside is that the same llvm will behave differently in the upstream compiler explorer, cannot be used for bugs, etc (since its behavior is different). In reality this means that mwe's built with this version will not have the same behavior usptream and thus not get properly triaged/fixed. If a flag didn't exist to set the rust-specific behavior, this would make a lot more sense, but that's not the issue here. The issue is turning changing enzyme to auto assume this for all code instead of just passing in the flag (specifically causing a contradiction between this repo and upstream, which will result in future debugging headaches and reducing support). A quick (though non exhaustive) search through the llvm rust fork divergence doesn't show any similar flag changes, but @nikic can likely weigh in further. |
|
@wsmoses Then how do we indicate that the flag is passed in the LLVMIR that we report to you? You are making this assumption based on a flag being passed or not but this is not visible in the emitted IR or Enzyme metadata as far as I can tell. The tools are made useless for bug reporting either way in the current situation. |
|
"Please just remember and make sure this is reported every time you report a bug" is making the assumption that every reporter is an expert anyway. |
|
I am fully on board with making it easier to produce issues (and like the design for attaching attributes/metadata in the IR to be checked for this). That is not what this PR does, which is why I am extremely hesitant about it. This PR changes the default of an existing flag. This flag is not set by any users at the moment, it is already set within rustc. Thus changing it here is meaningless. If users want to report an issue, they still need to include the flag, since that flag is needed by Enzyme proper (including our compiler explorer instance, which is the usual way bug reports [alongside logs] are being filed). This PR means there is a version of enzyme with the flag set the other way. Thus if users use this version of Enzyme they will get a file / issue which cannot be reproduced (and thus cannot be filed) on upstream Enzyme. Again, I am totally open to figuring out ways to make it easier to report bugs. The issue opened on Enzyme was closed by Manuel within 30 minutes after it seemed that his initial design (the same one here) wouldn't make sense to solve the issue. |
|
concurrently @ZuseZ4 I might recommend reading up on the XY problem (https://xyproblem.info/), it may be helpful for situations like this where you propose concretely "let's make the defaults cmake-configurable" instead of "how can we make it easier for users to file issues" |
|
I have a couple of objections/concerns, but I'm willing to skip them for now. |
|
if you still have concerns about the design, we should figure those about before implementing. but re an IR flag, sounds reasonable, I would likely recommend using a function attribute rather than a module flag to make it more configurable (and also more consistent with the other ta flag). Feel free to open a PR and I'll make sure it gets reviewed |
3 participants
Whether or not some IR is valid or not under strict-aliasing is determined by the language. Rust does not have the concept, so we want to set the default to false in https://github.com/EnzymeAD/Enzyme/blob/41b6c734483d62764381194aeb15b8690f8c99ee/enzyme/Enzyme/TypeAnalysis/TypeAnalysis.cpp#L93
It doesn't matter for rustc driven builds, but every debug build has to specify
-enzyme-strict-aliasing=0by hand, which doesn't make sense.@copilot add a cmake option (default off) to change the default of strict-aliasing from true to false, so that rust people loading enzyme via opt stop running into it.
Enzyme was unwilling to accept it in EnzymeAD#2885, but I care more about the concrete benefit to Rust devs willing to spend time debugging Enzyme, than about a potential future person downloading Enzyme via rustup, and then being confused that it used cmake to tune defaults in favour of Rust.