<div dir="ltr">Trying to sum-up the approaches that have been discussed, numbered in the order I saw them:<div><br></div><div>1) Mangle internal names to avoid collisions.</div><div><br></div><div>2) Only optimize library functions when they have external linkage.</div><div><br></div><div>3) Switch optimizations to do cloning rather than mutating functions</div><div><br></div><div>4) Mark all library functions declared in system headers with some attribute and key optimizations on this</div><div><br></div><div><br></div><div>#1 doesn't seem to have much appeal.</div><div>#3 is interesting and likely a good thing to do but not really sufficient to fix the root issue.</div><div>#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.</div><div><br></div><div>But I think we can combine some of #4 and some of #2 to get a good solution here that is practical and achievable:</div><div><br></div><div>- Recognize external library functions, much like we already do, but restrict it to external functions.</div><div>- Recognize internal functions *with a builtin attribute* much like we do external library functions.</div><div>- Teach internalize to add the builtin attribute as it changes linkage.</div><div><br></div><div>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.</div><div><br></div><div>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).</div><div><br></div><div>Thoughts?</div><div>-Chandler</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Sat, Nov 4, 2017 at 4:28 PM Chris Lattner via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><blockquote type="cite"><div>On Nov 4, 2017, at 3:12 PM, Alex Bradbury via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:</div><br class="m_-6326609745096932005Apple-interchange-newline"><div><div class="m_-6326609745096932005Singleton"><blockquote type="cite" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">I think this is the pragmatic way forwards. For a concise example of<br>how broken/surprising the current behaviour is:<br><snip><br>ffloor is legal for AArch64, meaning frintm is produced rather than a<br>call to floor. Deleting the 'readnone' attribute from the floor<br>function will avoid lowering to ffloor. Compile with -mtriple=arm and<br>the generated assembly has completely different semantics (calling<br>floor and so aborting).<br><br>I'm not sure if there's a tracking bug for this, but the earliest<br>mention I could find with a quick search was<br><<a href="https://bugs.llvm.org/show_bug.cgi?id=2141" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=2141</a>>.<br></blockquote><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">As John Regehr clarified on Twitter - the potential issues when</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">names+arguments clash with C99 standard library functions is</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">documented in the LangRef, though it's (at the time of writing)</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">stuffed awkwardly under the "Example" subheading for the call</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">instruction <</span><a href="http://llvm.org/docs/LangRef.html#id306" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" target="_blank">http://llvm.org/docs/LangRef.html#id306</a><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">>.</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">I suppose the point is: the issue described by Chandler in this RFC is</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">a very strong motivation for changing _something_. The approach</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">suggested by David would solve Chandler's bug, but also allow this</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">function naming restriction to be lifted altogether which seems like</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">an even bigger win.</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"></div></div></blockquote></div><br></div><div style="word-wrap:break-word"><div>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).</div><div><br></div><div>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..</div></div><div style="word-wrap:break-word"><div><br></div><div>-Chris</div><div><br></div></div>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>