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

[Rong Xu via llvm-commits]

OK. this can happen if the one icall-site has multiple promotions. the
exceeding won't happen cross the icall-site.

On Mon, Jul 11, 2016 at 10:57 AM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> 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/8e91bcd5/attachment.html>