<div dir="ltr">I guess I will remember not to write long scary comments next time ;)<div><br></div><div>David</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 26, 2016 at 2:35 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Rightio - thanks for walking me through it. Think I'm following/understand. Though the long comment you're adding confuses me in a few ways, but I'll write that off as lack of familiarity with this code & leave you to it.<br><br>(Guess there's no sane way to emit the counter code such that it just no-ops if the counter isn't linked in from the external def - that'd seem ideal, but I can't think of any linker/linkage tricks that would produce that behavior)<br><br>(please consider my issues closed & carry on with review from anyone else/more domain-specific reviewers)</div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 26, 2016 at 2:22 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, Feb 26, 2016 at 1:57 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, Feb 26, 2016 at 1:41 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, Feb 26, 2016 at 1:39 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, Feb 26, 2016 at 1:36 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, Feb 26, 2016 at 1:33 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, Feb 26, 2016 at 1:27 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, Feb 26, 2016 at 1:18 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, Feb 26, 2016 at 1:07 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, Feb 26, 2016 at 12:32 PM, David Li via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">davidxl created this revision.<br>
davidxl added a reviewer: vsk.<br>
davidxl added a subscriber: llvm-commits.<br>
<br>
The per function data and counter variables for available externally functions are created with linkonce linkage (to prevent compiler from dropping counter vars). However on ELF based systems, linkonce symbols are created as weak symbols and the duplicated entries are not removed by the linker. </blockquote><div><br></div></span><div>That ^ seems surprising/strange/confusing/problematic - any idea what's going on there? Is that a bug in the compiler? linker? specification of some kind?</div></div></div></div></blockquote><div><br></div></span><div>I am not sure what you questions is. Duplicate definitions of weak symbols are allowed so linker won't discard the duplicate copies. The symbol reference will be resolved the strong definition by the linker.</div></div></div></div></blockquote><div><br></div></span><div>Generally, I'm trying to understand why the counter is handled differently from the function itself.<br><br>The comment mentions "T<span style="color:rgb(80,0,80)">o avoid link errors, profile </span><span style="color:rgb(80,0,80)">counters for function with available_externally linkage needs to be changed t</span><span style="color:rgb(80,0,80)">o linkonce linkage.</span><span style="color:rgb(80,0,80)"> "<br>Why is that? Can we not rely on the counter to be emitted where the function is emitted? (where the external definition of the function is)</span></div><div><div><div> </div></div></div></div></div></div></blockquote><div><br></div></span><div>This is a good question. This is a design choice made long ago. For function with regular external linkage, the profile counter's linkage is set to 'internal', possibly to reduce the size overhead (by eliminating symbol table entries). Because of this, the available_external function's profile counter will no longer have a backup definition to be resolved to so their counter's linkage gets changed to linkonce.</div></div></div></div></blockquote></span><div><br>It seems like the compiler's in control of available_externally definitions, would it be possible that it would be in control of not internalizing counters it knows it may use available_externally elsewhere? (ie: when emitting the external definition of a thing that the compiler may emit available_externally definitions for, do not internalize the counter?)</div></div></div></div></blockquote><div><br></div></span><div>I am not aware of a way to check this condition -- if there is one I would certainly like to use it :)</div></div></div></div></blockquote></span></div></div></div></blockquote><div><br></div></span><div>I believe it's knowable - there may not be a single function that works as a perfect predicate for this now, but all I'm saying is: we/the compiler knows which entities we emit as available_externally, so we can know this when emitting the external definition too.</div></div></div></div></blockquote><div><br></div></span><div>I guess FE needs to pass that down to IR in some way.</div><span><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div><br></div></span><div>Actually there is another complicacy here -- user may decide not to instrument the module with the strong definition -- thus this can not really be depended upon.</div></div></div></div></blockquote><div><br></div></span><div>Do we support this, generally? (what do we deal with inline functions and their associated counter?)</div></div></div></div></blockquote><div><br></div></span><div>Yes, we do support this. We have customers with legit reason that some modules can not be instrumented. What is lost is only the counter values for the out of line copy.</div><div><br></div><div>A more general case is actually that the available_externally function is strongly defined in a standard library (libc++.so) for instance. The example I mentioned in the patch is such a case.</div><span><font color="#888888"><div><br></div><div>David</div></font></span><div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div><br></div><div>David</div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div><br></div><div>David</div></font></span><div><div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div><br></div><div>David</div></font></span><div><div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div><br></div><div>David</div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>For bad consequences it causes, see comments in the code. One example, the profile counts for _ZNKSs7_M_dataEv method is duplicated 655 times in raw profile data of clang. In the merged indexed profile, the counter values are magnified 655x -- totally dwarfed other functions -- this also distorted profile summary a lot leading to not useful profile data.<br>
<br>
<a href="http://reviews.llvm.org/D17654" rel="noreferrer" target="_blank">http://reviews.llvm.org/D17654</a><br>
<br>
Files:<br>
lib/Transforms/Instrumentation/InstrProfiling.cpp<br>
test/Instrumentation/InstrProfiling/linkage.ll<br>
<br>
Index: test/Instrumentation/InstrProfiling/linkage.ll<br>
===================================================================<br>
--- test/Instrumentation/InstrProfiling/linkage.ll<br>
+++ test/Instrumentation/InstrProfiling/linkage.ll<br>
@@ -7,6 +7,7 @@<br>
@__profn_foo_weak = weak hidden constant [8 x i8] c"foo_weak"<br>
@"__profn_linkage.ll:foo_internal" = internal constant [23 x i8] c"linkage.ll:foo_internal"<br>
@__profn_foo_inline = linkonce_odr hidden constant [10 x i8] c"foo_inline"<br>
+@__profn_foo_extern = linkonce_odr hidden constant [10 x i8] c"foo_extern"<br>
<br>
; COMMON: @__profc_foo = hidden global<br>
; COMMON: @__profd_foo = hidden global<br>
@@ -36,6 +37,15 @@<br>
ret void<br>
}<br>
<br>
+; LINUX: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_cnts", comdat($__profv_foo_extern), align 8<br>
+; LINUX: @__profd_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_data", comdat($__profv_foo_extern), align 8<br>
+; OTHER: @__profc_foo_extern = linkonce_odr hidden global<br>
+; OTHER: @__profd_foo_extern = linkonce_odr hidden global<br>
+define available_externally void @foo_extern() {<br>
+ call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_extern, i32 0, i32 0), i64 0, i32 1, i32 0)<br>
+ ret void<br>
+}<br>
+<br>
declare void @llvm.instrprof.increment(i8*, i64, i32, i32)<br>
<br>
; OTHER: @__llvm_profile_runtime = external global i32<br>
Index: lib/Transforms/Instrumentation/InstrProfiling.cpp<br>
===================================================================<br>
--- lib/Transforms/Instrumentation/InstrProfiling.cpp<br>
+++ lib/Transforms/Instrumentation/InstrProfiling.cpp<br>
@@ -286,8 +286,38 @@<br>
return F->hasAddressTaken();<br>
}<br>
<br>
-static inline Comdat *getOrCreateProfileComdat(Module &M,<br>
+static inline bool needsComdatForCounter(Function &F, Module &M) {<br>
+<br>
+ if (F.hasComdat())<br>
+ return true;<br>
+<br>
+ Triple TT(M.getTargetTriple());<br>
+ if (!TT.isOSBinFormatELF())<br>
+ return false;<br>
+<br>
+ // See createPGOFuncNameVar for more details. To avoid link errors, profile<br>
+ // counters for function with available_externally linkage needs to be changed<br>
+ // to linkonce linkage. On ELF based systems, this leads to weak symbols to be<br>
+ // created. Without using comdat, duplicate entries won't be removed by the<br>
+ // linker leading to increased data segement size and raw profile size. Even<br>
+ // worse, since the referenced counter from profile per-function data object<br>
+ // will be resolved to the common strong definition, the profile counts for<br>
+ // available_externally functions will end up being duplicated in raw profile<br>
+ // data. This can result in distorted profile as the counts of those dups<br>
+ // will be accumulated by the profile merger.<br>
+ GlobalValue::LinkageTypes Linkage = F.getLinkage();<br>
+ if (Linkage != GlobalValue::ExternalWeakLinkage &&<br>
+ Linkage != GlobalValue::AvailableExternallyLinkage)<br>
+ return false;<br>
+<br>
+ return true;<br>
+}<br>
+<br>
+static inline Comdat *getOrCreateProfileComdat(Module &M, Function &F,<br>
InstrProfIncrementInst *Inc) {<br>
+ if (!needsComdatForCounter(F, M))<br>
+ return nullptr;<br>
+<br>
// COFF format requires a COMDAT section to have a key symbol with the same<br>
// name. The linker targeting COFF also requires that the COMDAT<br>
// a section is associated to must precede the associating section. For this<br>
@@ -315,8 +345,7 @@<br>
// linking.<br>
Function *Fn = Inc->getParent()->getParent();<br>
Comdat *ProfileVarsComdat = nullptr;<br>
- if (Fn->hasComdat())<br>
- ProfileVarsComdat = getOrCreateProfileComdat(*M, Inc);<br>
+ ProfileVarsComdat = getOrCreateProfileComdat(*M, *Fn, Inc);<br>
<br>
uint64_t NumCounters = Inc->getNumCounters()->getZExtValue();<br>
LLVMContext &Ctx = M->getContext();<br>
<br>
<br>
<br></div></div>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>