<div dir="ltr">OK. this can happen if the one icall-site has multiple promotions. the exceeding won't happen cross the icall-site. <div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 11, 2016 at 10:57 AM, 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 class="">On Mon, Jul 11, 2016 at 10:50 AM, Rong Xu <span dir="ltr"><<a href="mailto:xur@google.com" target="_blank">xur@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">xur added inline comments.<br>
<br>
================<br>
<span>Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:262<br>
@@ -312,11 +261,3 @@<br>
<br>
-    if (ICPInvokeOnly && dyn_cast<CallInst>(Inst)) {<br>
-      DEBUG(dbgs() << " Not promote: User options.\n");<br>
-      break;<br>
-    }<br>
-    if (ICPCallOnly && dyn_cast<InvokeInst>(Inst)) {<br>
-      DEBUG(dbgs() << " Not promote: User option.\n");<br>
-      break;<br>
-    }<br>
     if (ICPCutOff != 0 && NumOfPGOICallPromotion >= ICPCutOff) {<br>
       DEBUG(dbgs() << " Not promote: Cutoff reached.\n");<br>
----------------<br>
</span><span>davidxl wrote:<br>
> I think there is a bug in the original code. This check should be moved after legality check below. The update of NumOfPGOICallPromotion should also be done here instead of in tryToPromote.<br>
><br>
> But this can be fixed in a different patch.<br>
</span>Which check are you referring to?<br>
These early exits seem good to me: for the *Only check, the icall type only depends on the instruction. it has nothing to do with legality.<br>
For the cutoff check, only it reaches, we can safely exit without doing the other checks. why you think this is a bug?<br>
<br></blockquote><div><br></div><div><div>    if (ICPCutOff != 0 && NumOfPGOICallPromotion >= ICPCutOff) {</div><div>       DEBUG(dbgs() << " Not promote: Cutoff reached.\n");</div></div><div><br></div><div><br></div></span><div>Here NumOfPGOICallPromotion does not get updated (it is delayed until transformation) so it is possible that the number of promotion candidates exceed the cutoff -- unless the cutoff is checked again in tryPromote.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>David</div></font></span><span class=""><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
================<br>
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:595<br>
@@ -661,1 +594,3 @@<br>
+    annotateValueSite(*M, *I, ICallProfDataRef.slice(NumPromoted), TotalCount,<br>
+                      IPVK_IndirectCallTarget, NumPromoted);<br>
   }<br>
----------------<br>
is the last argument correct? I think it should be NumCandidates.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D22182" rel="noreferrer" target="_blank">http://reviews.llvm.org/D22182</a><br>
<br>
<br>
<br>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div>