llvm dev - Oct 2017 - RFC: We need to explicitly state that some functions are reserved by LLVM

On 10/26/2017 10:56 PM, Chris Lattner via llvm-dev
wrote:
>
>> On Oct 26, 2017, at 8:14 PM, Chandler Carruth via llvm-dev 
>> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
>>
>>
>> One alternative that seems appealing but doesn't actually help
would
>> be to make `TargetLibraryInfo` ignore internal functions. That is how 
>> the C++ spec seems to handle this for example (C library function 
>> names are reserved only when they have linkage). But this doesn't 
>> work well for LLVM because we want to be able to LTO an internalized 
>> C library. So I think we need the rule for LLVM function names to not 
>> rely on linkage here.
>
> Oh sorry, (almost) TLDR I didn’t get to this part.  I don’t see how 
> this is applicable.  If you’re statically linking in a libc, I think 
> it is fine to forgo the optimizations that TargetLibraryInfo is all about.
>
> If these transformations are important to use in this case, we should 
> invent a new attribute, and the thing that turns libc symbols into 
> internal ones should add the attribute to the (now internal) libc symbols.
I'm not sure; some of the transformations are somewhat special (e.g., 
based on mathematical properties, or things like printf -> puts 
translation). LTO alone certainly won't give you those kinds of things 
via normal IPA, and I doubt we want attributes for all of them. Also, 
having LTO essentially disable optimizations isn't good either.

  -Hal
>
> -Chris
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171026/6f3631a0/attachment.html>

On 10/26/2017 9:01 PM, Hal Finkel via llvm-dev wrote:
>
>
> On 10/26/2017 10:56 PM, Chris Lattner via llvm-dev wrote:
>>
>>> On Oct 26, 2017, at 8:14 PM, Chandler Carruth via llvm-dev 
>>> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
>>>
>>>
>>> One alternative that seems appealing but doesn't actually help
would
>>> be to make `TargetLibraryInfo` ignore internal functions. That is 
>>> how the C++ spec seems to handle this for example (C library 
>>> function names are reserved only when they have linkage). But this 
>>> doesn't work well for LLVM because we want to be able to LTO an
>>> internalized C library. So I think we need the rule for LLVM 
>>> function names to not rely on linkage here.
>>
>> Oh sorry, (almost) TLDR I didn’t get to this part.  I don’t see how 
>> this is applicable.  If you’re statically linking in a libc, I think 
>> it is fine to forgo the optimizations that TargetLibraryInfo is all 
>> about.
>>
>> If these transformations are important to use in this case, we should 
>> invent a new attribute, and the thing that turns libc symbols into 
>> internal ones should add the attribute to the (now internal) libc 
>> symbols.
>
> I'm not sure; some of the transformations are somewhat special (e.g., 
> based on mathematical properties, or things like printf -> puts 
> translation). LTO alone certainly won't give you those kinds of things 
> via normal IPA, and I doubt we want attributes for all of them. Also, 
> having LTO essentially disable optimizations isn't good either.
Given the way the optimization pipeline works; we can't treat an 
"internal" function as equivalent to a C library function.  When the 
linkage of a function becomes "internal", optimizations start kicking
in
based on the fact that we can see all the users of the function.

For example, suppose my program has one call to puts with the constant 
string "foo", and one call to printf which can be transformed into a 
call to puts, and we LTO the C library into it. First we run IPSCCP, 
which will constant-propagate the address of the string into the 
implementation of puts.  Then instcombine runs and transforms the call 
to printf into a call to puts.  Now we have a miscompile, because our 
"puts" can only output "foo".

Given we have mutually exclusive optimizations, we have to pick: either 
we allow the IPSCCP transform, or we allow the instcombine transform.  
The most straightforward way to indicate the difference is to check the 
linkage: it intuitively has the right meaning, and our existing 
inter-procedural optimizations already check it.

-Eli

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171027/1afb2a67/attachment.html>

On 10/27/2017 01:36 PM, Friedman, Eli wrote:
> On 10/26/2017 9:01 PM, Hal Finkel via llvm-dev wrote:
>>
>>
>> On 10/26/2017 10:56 PM, Chris Lattner via llvm-dev wrote:
>>>
>>>> On Oct 26, 2017, at 8:14 PM, Chandler Carruth via llvm-dev 
>>>> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
>>>>
>>>>
>>>> One alternative that seems appealing but doesn't actually
help
>>>> would be to make `TargetLibraryInfo` ignore internal functions.
>>>> That is how the C++ spec seems to handle this for example (C 
>>>> library function names are reserved only when they have
linkage).
>>>> But this doesn't work well for LLVM because we want to be
able to
>>>> LTO an internalized C library. So I think we need the rule for
LLVM
>>>> function names to not rely on linkage here.
>>>
>>> Oh sorry, (almost) TLDR I didn’t get to this part.  I don’t see how
>>> this is applicable.  If you’re statically linking in a libc, I
think
>>> it is fine to forgo the optimizations that TargetLibraryInfo is all
>>> about.
>>>
>>> If these transformations are important to use in this case, we 
>>> should invent a new attribute, and the thing that turns libc
symbols
>>> into internal ones should add the attribute to the (now internal) 
>>> libc symbols.
>>
>> I'm not sure; some of the transformations are somewhat special
(e.g.,
>> based on mathematical properties, or things like printf -> puts 
>> translation). LTO alone certainly won't give you those kinds of 
>> things via normal IPA, and I doubt we want attributes for all of 
>> them. Also, having LTO essentially disable optimizations isn't good
>> either.
>
> Given the way the optimization pipeline works; we can't treat an 
> "internal" function as equivalent to a C library function.  When
the
> linkage of a function becomes "internal", optimizations start
kicking
> in based on the fact that we can see all the users of the function.
>
> For example, suppose my program has one call to puts with the constant 
> string "foo", and one call to printf which can be transformed
into a
> call to puts, and we LTO the C library into it.  First we run IPSCCP, 
> which will constant-propagate the address of the string into the 
> implementation of puts.  Then instcombine runs and transforms the call 
> to printf into a call to puts.  Now we have a miscompile, because our 
> "puts" can only output "foo".
>
> Given we have mutually exclusive optimizations, we have to pick: 
> either we allow the IPSCCP transform, or we allow the instcombine 
> transform.  The most straightforward way to indicate the difference is 
> to check the linkage: it intuitively has the right meaning, and our 
> existing inter-procedural optimizations already check it.
Good point. If we optimize a function on the basis of being able to see 
all users, it can no longer be "special" in this regard (and we also 
can't introduce new calls to it). In the general case (which, I imagine 
is the libc-LTO case), you might need to clone such a function when we 
specialize so that we can continue to introduce new calls to the 
original function (and  then DCE afterward).

  -Hal
>
> -Eli
> -- 
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171027/abfce069/attachment.html>

On Fri, Oct 27, 2017 at 11:36 AM, Friedman, Eli via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On 10/26/2017 9:01 PM, Hal Finkel via llvm-dev wrote:
>
>
> On 10/26/2017 10:56 PM, Chris Lattner via llvm-dev wrote:
>
>
> On Oct 26, 2017, at 8:14 PM, Chandler Carruth via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>
> One alternative that seems appealing but doesn't actually help would be
to
> make `TargetLibraryInfo` ignore internal functions. That is how the C++
> spec seems to handle this for example (C library function names are
> reserved only when they have linkage). But this doesn't work well for
LLVM
> because we want to be able to LTO an internalized C library. So I think we
> need the rule for LLVM function names to not rely on linkage here.
>
>
> Oh sorry, (almost) TLDR I didn’t get to this part.  I don’t see how this
> is applicable.  If you’re statically linking in a libc, I think it is fine
> to forgo the optimizations that TargetLibraryInfo is all about.
>
> If these transformations are important to use in this case, we should
> invent a new attribute, and the thing that turns libc symbols into internal
> ones should add the attribute to the (now internal) libc symbols.
>
>
> I'm not sure; some of the transformations are somewhat special (e.g.,
> based on mathematical properties, or things like printf -> puts
> translation). LTO alone certainly won't give you those kinds of things
via
> normal IPA, and I doubt we want attributes for all of them. Also, having
> LTO essentially disable optimizations isn't good either.
>
>
> Given the way the optimization pipeline works; we can't treat an
> "internal" function as equivalent to a C library function.  When
the
> linkage of a function becomes "internal", optimizations start
kicking in
> based on the fact that we can see all the users of the function.
>
> For example, suppose my program has one call to puts with the constant
> string "foo", and one call to printf which can be transformed
into a call
> to puts, and we LTO the C library into it.  First we run IPSCCP, which will
> constant-propagate the address of the string into the implementation of
> puts.  Then instcombine runs and transforms the call to printf into a call
> to puts.  Now we have a miscompile, because our "puts" can only
output
> "foo".
>

Slightly off topic. This particular example probably just shows the issue
in implementation. A more robust way to implement this is to 'clone' the
original function and privatize it instead of the original function.  The
removal of the original function can be delayed till the real link time
when linker sees no references to it.   This is also a more flexible scheme
as LTO does not need to operate in strict whole program mode, nor does it
need to query linker to see if the function is referenced by other library
functions not visible to the compiler (in IR form), or there is reference
to the symbol through dlopen/dlsym ..

David


>
> Given we have mutually exclusive optimizations, we have to pick: either we
> allow the IPSCCP transform, or we allow the instcombine transform.  The
> most straightforward way to indicate the difference is to check the
> linkage: it intuitively has the right meaning, and our existing
> inter-procedural optimizations already check it.
>
> -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/20171028/b810e961/attachment.html>