llvm dev - Nov 2017 - 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>

On Nov 10, 2017, at 7:54 PM, Chandler Carruth <chandlerc at gmail.com>
wrote:
> 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).
This approach seems like it would work to me!

The only challenge is that you’ll need a list of “known builtins” that
internalize can use, that doesn’t seem like a bad thing though.

-Chris

Internalize can just use target lib info since it sees them still external.

On Sat, Nov 11, 2017, 11:47 Chris Lattner <clattner at nondot.org> wrote:
> On Nov 10, 2017, at 7:54 PM, Chandler Carruth <chandlerc at
gmail.com> wrote:
> > 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).
>
> This approach seems like it would work to me!
>
> The only challenge is that you’ll need a list of “known builtins” that
> internalize can use, that doesn’t seem like a bad thing though.
>
> -Chris
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171111/c5c0f8e4/attachment.html>

On 11/10/2017 7:54 PM, Chandler Carruth via llvm-dev
wrote:
> 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.
If we're LTO'ing in an entire libc, there are certain functions which 
are special in weird ways, which I'm not sure we can represent properly 
with your suggested representation.  ISel can generate calls to C 
library functions (everything in RuntimeLibcalls.def, including 
memcpy/memset/memmove and a bunch of libm functions).  And the
"noalias"
attribute on malloc() depends on the fact that we can't actually see the 
implementation normally.  But maybe we can work on that incrementally?

Also, as a side-note, -fno-math-errno isn't really a great example. The 
fact that we add readnone to math functions which can set errno is a 
bug; we just haven't fixed it because speed is more important than 
correctness in fast-math mode, and nobody has implemented a suitable 
alternative.  (It's easy to write a testcase where we miscompile because 
a math function clobbers the errno set by some other function.)

-Eli

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project

On Mon, Nov 13, 2017 at 11:25 AM Friedman, Eli <efriedma at
codeaurora.org>
wrote:
> On 11/10/2017 7:54 PM, Chandler Carruth via llvm-dev wrote:
> > 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.
>
> If we're LTO'ing in an entire libc, there are certain functions
which
> are special in weird ways, which I'm not sure we can represent properly
> with your suggested representation.  ISel can generate calls to C
> library functions (everything in RuntimeLibcalls.def, including
> memcpy/memset/memmove and a bunch of libm functions).  And the
"noalias"
> attribute on malloc() depends on the fact that we can't actually see
the
> implementation normally.  But maybe we can work on that incrementally?
>
Yeah, there are a lot of issues that start to crop up here. But my hope is
exactly what you say: we should handle that incrementally.

> Also, as a side-note, -fno-math-errno isn't really a great example. The
> fact that we add readnone to math functions which can set errno is a
> bug; we just haven't fixed it because speed is more important than
> correctness in fast-math mode, and nobody has implemented a suitable
> alternative.  (It's easy to write a testcase where we miscompile
because
> a math function clobbers the errno set by some other function.)
>
Sure, but I think it makes the point.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171113/bc775197/attachment.html>

On Mon, Nov 13, 2017 at 11:25 AM, Friedman, Eli via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On 11/10/2017 7:54 PM, Chandler Carruth via llvm-dev wrote:
>
>> 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.
>>
>
> If we're LTO'ing in an entire libc, there are certain functions
which are
> special in weird ways, which I'm not sure we can represent properly
with
> your suggested representation.  ISel can generate calls to C library
> functions (everything in RuntimeLibcalls.def, including
> memcpy/memset/memmove and a bunch of libm functions).  And the
"noalias"
> attribute on malloc() depends on the fact that we can't actually see
the
> implementation normally.  But maybe we can work on that incrementally?
>

Note that there is option -fno-builtin-malloc for that, but there seems to
be a bug in implementation -- with the flag, the noalias attribute is still
applied which is wrong.

>
> Also, as a side-note, -fno-math-errno isn't really a great example. The
> fact that we add readnone to math functions which can set errno is a bug;
> we just haven't fixed it because speed is more important than
correctness
> in fast-math mode, and nobody has implemented a suitable alternative.
> (It's easy to write a testcase where we miscompile because a math
function
> clobbers the errno set by some other function.)

Yes, it is a quality of implementation issue. If the side-effect/mod-ref
representation in IR allows more fine grained control, e.g, allowing
specifying the function only touches errno or otherwise side-effect free,
then the impact on performance will be minimal.

David

>
>
> -Eli
>
> --
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> Foundation Collaborative Project
>
> _______________________________________________
> 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/20171113/d15b6934/attachment.html>