[PATCH] D22182: Refactor indirect call promotion profitability analysis (NFC)

[Xinliang David Li via llvm-commits]

On Mon, Jul 11, 2016 at 10:50 AM, Rong Xu <xur at google.com> wrote:

> xur added inline comments.
>
> ================
> Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:262
> @@ -312,11 +261,3 @@
>
> -    if (ICPInvokeOnly && dyn_cast<CallInst>(Inst)) {
> -      DEBUG(dbgs() << " Not promote: User options.\n");
> -      break;
> -    }
> -    if (ICPCallOnly && dyn_cast<InvokeInst>(Inst)) {
> -      DEBUG(dbgs() << " Not promote: User option.\n");
> -      break;
> -    }
>      if (ICPCutOff != 0 && NumOfPGOICallPromotion >= ICPCutOff) {
>        DEBUG(dbgs() << " Not promote: Cutoff reached.\n");
> ----------------
> davidxl wrote:
> > 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.
> >
> > But this can be fixed in a different patch.
> Which check are you referring to?
> 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.
> For the cutoff check, only it reaches, we can safely exit without doing
> the other checks. why you think this is a bug?
>
>
    if (ICPCutOff != 0 && NumOfPGOICallPromotion >= ICPCutOff) {
       DEBUG(dbgs() << " Not promote: Cutoff reached.\n");


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.

David




> ================
> Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:595
> @@ -661,1 +594,3 @@
> +    annotateValueSite(*M, *I, ICallProfDataRef.slice(NumPromoted),
> TotalCount,
> +                      IPVK_IndirectCallTarget, NumPromoted);
>    }
> ----------------
> is the last argument correct? I think it should be NumCandidates.
>
>
> http://reviews.llvm.org/D22182
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160711/d4c8ef6a/attachment.html>