-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Doing certain kinds of IPO over comdat functions is unsound #27148
Comments
Adding a link to a complete reproducer (also from Sanjoy) that has nothing to do with atomics: |
FYI: that's the zip file I've attached to this bug. |
I vote that we stop inferring readnone/readonly/etc on functions with isWeakForLinker linkage. I expect that in most places where we can infer such attributes, inlining is likely to happen anyway, and be just as profitable. Are we aware of any other IPO soundness issues with other passes that aren't functionattrs? I think IPSCP is fine for ODR linkage functions, they all have to return the same constant regardless of optimization. |
Just a note: be careful with isWeakForLinker(), it is not the right one (as I wrote in the email thread). I think the right condition is !isStrongDefinitionForLinker(). |
I think both ipsccp and ipconstprop are bad. Consider a variant of void f(i32* %ptr) available_externally { void caller() { function local opts ==> void f(i32* %ptr) available_externally { void caller() { ipsccp or ipconstprop ==> void f(i32* %ptr) available_externally { void caller() { Now if we link to the unoptimized @f, we can print {, I think (haven't tried) we can construct similar cases with undef |
I think we need to be careful here; you could be right, but there seem to be some attributes that we do commonly prove for inline functions, even those too large to inline. We can prove 'nocapture' on the implicit this pointer, for example. readonly seems more common than readnone. I really think we need to benchmark this against privatizing the functions to make an informed decision.
I suspect that GlobalsAA has the same problem as attribute propagation (in one TU, a function's access to a global could be optimized out, but the linker could pick a version of the function which still accesses the global). [And privatizing just got harder?...] |
I see a change was made at r265762 to address this issue. That's causing a |
mentioned in issue llvm/llvm-bugzilla-archive#27796 |
Extended Description
This was discussed on llvm-dev: http://lists.llvm.org/pipermail/llvm-dev/2016-February/095833.html
Description copy-pasted from the thread:
"""
Let's start with an example that shows that we have a problem (direct
copy/paste from the guard intrinsics thread). Say we have:
The possible behaviors of the above program are {print("X"),
print("Y")} or {print("Y")}. But if we run opt then we have
and some generic reordering
If we do not inline @foo(), and instead re-link the call site in @main
to some non-optimized copy (or differently optimized copy) of @foo,
then it is possible for the program to have the behavior {print("Y");
print ("X")}, which was disallowed in the earlier program.
In other words, opt refined the semantics of @foo() (i.e. reduced the
set of behaviors it may have) in ways that would make later
optimizations invalid if we de-refine the implementation of @foo().
The above example is clearly fabricated, but such cases can come up
even if everything is optimized to the same level. E.g. one of the
atomic loads in the unrefined implementation of @foo() could have been
hidden behind a function call, whose body existed in only one module.
That module would then be able to refine @foo() to
ret void
butother modules won't.
"""
The text was updated successfully, but these errors were encountered: