<div dir="ltr"><div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 17, 2019 at 4:25 PM Rong Xu <<a href="mailto:xur@google.com">xur@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">+cc David<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 17, 2019 at 3:20 PM Reid Kleckner via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">rnk added a comment.<br>
<br>
>From what I understand, the comdat group isn't necessary, so I disabled it on COFF in r372182. Can we simplify ELF to match?<br></blockquote></div></div></blockquote><div><br></div><div>Putting prof sections with the function in the same comdat group is probably not needed. There might be some weird interaction with GC and instrumentation binary size. If we don't see issues there, it is ok to simplify ELF to match.  Rong can help with some internal testing (instr build size, performance etc) on internal large app with linux. </div><div><br></div><div>David</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"></blockquote><div>It's probably OK without setting COMDAT for the variables in ELF as the linkonce_odr  linkage can be used to remove the duplicates. </div><div>On the other hand, it's nature to set the COMDAT to sync the declaration of the function. </div><div>I'm CCing david to see what is his thoughts.</div><div><br></div><div>-Rong</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D67579/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D67579/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D67579" rel="noreferrer" target="_blank">https://reviews.llvm.org/D67579</a><br>
<br>
<br>
<br>
</blockquote></div></div>
</blockquote></div></div>