Alex Bradbury via llvm-dev
2017-Nov-04 22:12 UTC
[llvm-dev] RFC: We need to explicitly state that some functions are reserved by LLVM
On 4 November 2017 at 20:58, Alex Bradbury <asb at asbradbury.org> wrote:> On 27 October 2017 at 19:31, Hal Finkel via llvm-dev > <llvm-dev at lists.llvm.org> wrote: >> I agree. Marking external functions from system headers seems like a >> reasonable heuristic. We'd need some heuristic because it's not reasonable >> for the frontend to know about every function the optimizer knows about. >> Over-marking seems okay, however. > > I think this is the pragmatic way forwards. For a concise example of > how broken/surprising the current behaviour is: > <snip> > ffloor is legal for AArch64, meaning frintm is produced rather than a > call to floor. Deleting the 'readnone' attribute from the floor > function will avoid lowering to ffloor. Compile with -mtriple=arm and > the generated assembly has completely different semantics (calling > floor and so aborting). > > I'm not sure if there's a tracking bug for this, but the earliest > mention I could find with a quick search was > <https://bugs.llvm.org/show_bug.cgi?id=2141>.As John Regehr clarified on Twitter - the potential issues when names+arguments clash with C99 standard library functions is documented in the LangRef, though it's (at the time of writing) stuffed awkwardly under the "Example" subheading for the call instruction <http://llvm.org/docs/LangRef.html#id306>. I suppose the point is: the issue described by Chandler in this RFC is a very strong motivation for changing _something_. The approach suggested by David would solve Chandler's bug, but also allow this function naming restriction to be lifted altogether which seems like an even bigger win. Best, Alex
Chris Lattner via llvm-dev
2017-Nov-04 23:28 UTC
[llvm-dev] RFC: We need to explicitly state that some functions are reserved by LLVM
> On Nov 4, 2017, at 3:12 PM, Alex Bradbury via llvm-dev <llvm-dev at lists.llvm.org> wrote: > >> I think this is the pragmatic way forwards. For a concise example of >> how broken/surprising the current behaviour is: >> <snip> >> ffloor is legal for AArch64, meaning frintm is produced rather than a >> call to floor. Deleting the 'readnone' attribute from the floor >> function will avoid lowering to ffloor. Compile with -mtriple=arm and >> the generated assembly has completely different semantics (calling >> floor and so aborting). >> >> I'm not sure if there's a tracking bug for this, but the earliest >> mention I could find with a quick search was >> <https://bugs.llvm.org/show_bug.cgi?id=2141 <https://bugs.llvm.org/show_bug.cgi?id=2141>>. > > As John Regehr clarified on Twitter - the potential issues when > names+arguments clash with C99 standard library functions is > documented in the LangRef, though it's (at the time of writing) > stuffed awkwardly under the "Example" subheading for the call > instruction <http://llvm.org/docs/LangRef.html#id306 <http://llvm.org/docs/LangRef.html#id306>>. > > I suppose the point is: the issue described by Chandler in this RFC is > a very strong motivation for changing _something_. The approach > suggested by David would solve Chandler's bug, but also allow this > function naming restriction to be lifted altogether which seems like > an even bigger win.I think that the right thing to do is to make the compiler ignore well-known functions that have internal linkage. Treating a symbol with internal linkage as “known” is unsafe and incorrect even if it was derived from a well-known function, because IPO can transform it (e.g. by constant propagating values into the arguments). If the use-case for statically linking in libc + internalizing it is important, then we need to find another solution to preserve those optimizations, it isn’t safe to just blindly assume an internal symbol with a well known name is the well known function.. -Chris -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20171104/b5a9c6fa/attachment-0001.html>
Chandler Carruth via llvm-dev
2017-Nov-11 03:54 UTC
[llvm-dev] RFC: We need to explicitly state that some functions are reserved by LLVM
Trying to sum-up the approaches that have been discussed, numbered in the order I saw them: 1) Mangle internal names to avoid collisions. 2) Only optimize library functions when they have external linkage. 3) Switch optimizations to do cloning rather than mutating functions 4) Mark all library functions declared in system headers with some attribute and key optimizations on this #1 doesn't seem to have much appeal. #3 is interesting and likely a good thing to do but not really sufficient to fix the root issue. #4, especially in the mode w here these attributes actually carry the semantics allowing the name-based heuristics to be isolated in a more appropriate layer, seems like a very interesting long term path, but honestly not one I have the time to bring about right now. And I don't think we can wait for this to fix things. But I think we can combine some of #4 and some of #2 to get a good solution here that is practical and achievable: - Recognize external library functions, much like we already do, but restrict it to external functions. - Recognize internal functions *with a builtin attribute* much like we do external library functions. - Teach internalize to add the builtin attribute as it changes linkage. One example of what I *really* want from this even in LTO which motivates the change to internalize: things like 'readonly' where some spec lets us optimize callers with this even if the implementation actually writes to memory. Consider building with -fno-math-errno and LTOing a libc that does actually set errno in its implementation. We will also need to constrain optimizations like IPSCCP in the face of internal builtin (and thus library) functions in order to avoid the printf -> puts miscompile described by Eli. But we already have this problem in theory today, and the above won't make it any worse and should even give us new options to address it such as stripping the builtin attribute (in addition to cloning, or other techniques). Thoughts? -Chandler On Sat, Nov 4, 2017 at 4:28 PM Chris Lattner via llvm-dev < llvm-dev at lists.llvm.org> wrote:> > On Nov 4, 2017, at 3:12 PM, Alex Bradbury via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > > I think this is the pragmatic way forwards. For a concise example of > how broken/surprising the current behaviour is: > <snip> > ffloor is legal for AArch64, meaning frintm is produced rather than a > call to floor. Deleting the 'readnone' attribute from the floor > function will avoid lowering to ffloor. Compile with -mtriple=arm and > the generated assembly has completely different semantics (calling > floor and so aborting). > > I'm not sure if there's a tracking bug for this, but the earliest > mention I could find with a quick search was > <https://bugs.llvm.org/show_bug.cgi?id=2141>. > > > As John Regehr clarified on Twitter - the potential issues when > names+arguments clash with C99 standard library functions is > documented in the LangRef, though it's (at the time of writing) > stuffed awkwardly under the "Example" subheading for the call > instruction <http://llvm.org/docs/LangRef.html#id306>. > > I suppose the point is: the issue described by Chandler in this RFC is > a very strong motivation for changing _something_. The approach > suggested by David would solve Chandler's bug, but also allow this > function naming restriction to be lifted altogether which seems like > an even bigger win. > > > I think that the right thing to do is to make the compiler ignore > well-known functions that have internal linkage. Treating a symbol with > internal linkage as “known” is unsafe and incorrect even if it was derived > from a well-known function, because IPO can transform it (e.g. by constant > propagating values into the arguments). > > If the use-case for statically linking in libc + internalizing it is > important, then we need to find another solution to preserve those > optimizations, it isn’t safe to just blindly assume an internal symbol with > a well known name is the well known function.. > > -Chris > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20171111/20e83dfd/attachment.html>