<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Feb 26, 2016 at 7:59 PM Hal Finkel <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><b>From: </b>"Chandler Carruth" <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>><br><b>To: </b>"Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>><br><b>Cc: </b>"llvm-dev" <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>, "Philip Reames" <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>>, "Duncan P. N. Exon Smith" <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>>, "Xinliang David Li" <<a href="mailto:xinliangli@gmail.com" target="_blank">xinliangli@gmail.com</a>>, "Sanjoy Das" <<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.com</a>><br></blockquote></div></div><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><b>Sent: </b>Friday, February 26, 2016 9:41:23 PM</blockquote></div></div><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><br><b>Subject: </b>Re: [llvm-dev] Possible soundness issue with available_externally (split from "RFC: Add guard intrinsics")<br><br><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Feb 26, 2016 at 7:38 PM Hal Finkel <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><b>From: </b>"Chandler Carruth" <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>><br><b>To: </b>"Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>><br></blockquote></div></div><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><b>Cc: </b>"llvm-dev" <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>, "Philip Reames" <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>>, "Duncan P. N. Exon Smith" <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>>, "Xinliang David Li" <<a href="mailto:xinliangli@gmail.com" target="_blank">xinliangli@gmail.com</a>>, "Sanjoy Das" <<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.com</a>><br><b>Sent: </b>Friday, February 26, 2016 9:33:55 PM</blockquote></div></div><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><br><b>Subject: </b>Re: [llvm-dev] Possible soundness issue with available_externally (split from "RFC: Add guard intrinsics")<br><br></blockquote></div></div><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Feb 26, 2016 at 7:26 PM Hal Finkel <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>> wrote:<br></div></div></div></blockquote></div></div><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
> From: "Chandler Carruth" <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>><br>
> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>>, "Sanjoy Das" <<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.com</a>><br>
> Cc: "llvm-dev" <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>, "Philip Reames" <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>>, "Duncan P. N. Exon Smith"<br>
> <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>>, "Xinliang David Li" <<a href="mailto:xinliangli@gmail.com" target="_blank">xinliangli@gmail.com</a>><br>
> Sent: Friday, February 26, 2016 9:01:48 PM<br>
> Subject: Re: [llvm-dev] Possible soundness issue with available_externally (split from "RFC: Add guard intrinsics")<br>
><br>
><br>
> I think this will have a much higher cost than my proposal to<br>
> constrain how we deduce function attributes (which still fixes<br>
> Sanjoy's latest example).<br>
><br>
> Specifically, I think this will force us to constrain far too many<br>
> transformations for the sake of code size in functions that we won't<br>
> inline. Even if we were never going to deduce function attributes<br>
> for anything in the function (because its big and reads and writes<br>
> everything), we'll still have to constrain our transformations just<br>
> because we *might* later deduce a function attribute that triggers<br>
> these kinds of bugs.<br>
><br>
> Essentially, you're proposing to limit intraprocedural optimization<br>
> to when we can successfully to interprocedural optimization<br>
> ("privatization"), where I'm suggesting we limit interprocedural<br>
> optimization to leave intraprocedural optimization unconstrained.<br>
> Given the ratio of our optimizations (almost all are intra, very few<br>
> are inter), I'm much more comfortable with the latter.<br>
<br>
This is a good point; we can certainly (easily) delay the privatization decision until we modify any IPA-level function information (at which point we can either reject the attribute change (when optimizing for code size), or keep it locally (when optimizing for speed). Ideally, you'd want to delay this even further (until you knew the attribute information was used), but I'm not sure that's practical.<br>
<br>
Actually, what if instead of actually privatizing, we moved the function into a different comdat section named after some hash of the function body? That way, if all versions are actually optimized identically, we'll still only end up with one copy in the final executable. If this is technically possible, it seems like the best kind of solution.<br></blockquote></div></div></blockquote></div></div><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>This is how I want to do a revamped function merging anyways and it would fall out naturally of that.</div></div></div></blockquote></div></div><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div class="gmail_quote"></div></div>
</blockquote>Excellent, so let's fix this at the same time we implement this function merging (so that we don't have performance regressions in an intermediate state). This will also allow us to have uniform logic across different optimization levels, which is obviously preferable.<br></div></div></blockquote><div><br></div><div>I am *extremely* uncomfortable waiting to fix this until merging stuff is in place and we add privatization heuristics to our IPO passes. Those might be years away.</div></div></div></blockquote></div></div><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"></blockquote>I'm *extremely* uncomfortable fixing this at all unless it can be done without causing performance regressions. The underlying basic use case (linking together code compiled with different optimization levels), is certainly something I'd like to work properly, but is definitely a far lower priority than optimized code quality and size.<br></div></div></blockquote><div><br></div><div>Ok, I prioritize this differently: it is absolutely critical. Every binary I have includes files that fit this description. Every single binary.</div><div><br></div><div>I think that we will discover subtle miscompiles every time we try to make function attributes more powerful until this is fixed. I think this is absolutely critical to fix.</div><div><br></div><div>I'm fine if you want an option to enable unsafe behavior here, but I think we have to have an option that provides correct results and I frankly think it should be the default.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><br>Furthermore, you agree that there is a technical solution that satisfies these requirements (placing functions into their own hashed comdat sections),</div></div></blockquote><div><br></div><div>No, I think that is a significant overstatement. I think that this would require a pretty sizable amount of work to get the size regression to be tolerable. I don't know how much work, nor when it could be done. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"> and I don't see why this is not relatively-straightforward to implement, and so if we want to fix this bug, we should implement it. In the common case (where everything is optimized at the same level), I don't see why it has any additional overhead. We should be able to privatize aggressively under this scheme.</div></div></blockquote><div><br></div><div>I don't have any confidence in our ability to perfectly merge privatized routines. Maybe you do, but I genuinely do not how to do it with sufficient reliability today.</div><div><br></div><div>I have ideas about how to get a *good* approximation that I think will recover much of the penalty. But I think it would still need a size threshold....</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><br><br> -Hal</div></div><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><br><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div class="gmail_quote"><div></div><div><br></div><div>I think we should pretty immediately:</div><div><br></div><div>1) Make our IPO passes conservatively correc</div><div>2) Leave comments about how to add privatization, but explain the code size cost incurred by it</div><div><br></div><div>I have no idea if anyone is even working on privatization (I'm not) or function merging (no ETA at all, its really far down my queue). I think we should decouple all these pieces. Once things are correct, folks can add a very size-conservative privatization transformation to IPO routines if we don't have merging, or a fairly aggressive one if we do have merging. And if we add merging later we can re-tune the privatization.</div><div><br></div><div>-Chandler</div><div> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><br>Thanks again,<br>Hal</div></div><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><br><br>-- <br><div><span></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span></span><br></div></div></div></blockquote></div></div>
</blockquote><br><br><br>-- <br><div><span name="x"></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></div></div></blockquote></div></div>