<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 12, 2017 at 3:17 PM, Reid Kleckner via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">rnk added a comment.<br>
<br>
Are we sure we want it like this? Doesn't this retain instrprof data structures from both A.obj and B.obj for inline functions present in both objs? Won't that be a problem?<br>
<br>
>> From @davidxl :<br>
<span class="">><br>
> It was comdat associated with the function before -- but it was wrong. The problem is that different comdat functions may have different CFGs (due to different early inlining), leading to incompatible profile counter data. Note that a comdat function can be later inlined, so using a shared copy of profile counter will be bad (out of bound access, not to mention profile use phase can not use it).<br>
<br>
</span>I agree, if instrprof runs before inlining, then we don't want these to be in a comdat group with the function. It's not safe for other functions to reference the data after inlining.<br>
<br>
However, if instrprof runs after "early inlining", which I didn't think we had any of, then how have we solved the problem of different comdat functions having different CFGs at instrumentation time?<br></blockquote><div><br></div><div>We have instrprof running after early inlining for IR based instrumentation.  The comdat key is created using cfg hashing so that comdat counters from the same comdat function are partially merged -- comdat instances sharing the same cfg will share the same counter.</div><div><br></div><div>David </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D32688" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D32688</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>