[PATCH] D18568: [PGO] Use ArrayRef in annotateValueSite()

[Rong Xu via llvm-commits]

David, thanks for the suggestion!  The newer patch uses makeArrayRef as
Sean had the same suggestion.
I'll drop the useless "const" in the parameter.

Thanks!

-Rong



On Tue, Mar 29, 2016 at 10:31 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Tue, Mar 29, 2016 at 12:49 PM, Rong Xu via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> xur created this revision.
>> xur added reviewers: silvas, davidxl.
>> xur added subscribers: xur, llvm-commits.
>>
>> Using ArrayRef in annotateValueSite's parameter instead of using an array
>> and it's size. This is one of cleanups suggested by Sean Silva in
>> http://reviews.llvm.org/D17864.
>>
>> http://reviews.llvm.org/D18568
>>
>> Files:
>>   include/llvm/ProfileData/InstrProf.h
>>   lib/ProfileData/InstrProf.cpp
>>   unittests/ProfileData/InstrProfTest.cpp
>>
>> Index: unittests/ProfileData/InstrProfTest.cpp
>> ===================================================================
>> --- unittests/ProfileData/InstrProfTest.cpp
>> +++ unittests/ProfileData/InstrProfTest.cpp
>> @@ -330,7 +330,9 @@
>>    // Annotate with 4 records.
>>    InstrProfValueData VD0Sorted[] = {{1000, 6}, {2000, 5}, {3000, 4},
>> {4000, 3},
>>                                {5000, 2}, {6000, 1}};
>> -  annotateValueSite(*M, *Inst, &VD0Sorted[2], 4, 10,
>> IPVK_IndirectCallTarget, 5);
>> +  ArrayRef<InstrProfValueData> VDArrayRef(VD0Sorted, 6);
>>
>
> Can you just use makeArrayRef here ^ (so you don't have to duplicate the
> array length)? (or at least use array_lengthof rather than hardcoding it)
>
>
>> +  annotateValueSite(*M, *Inst, VDArrayRef.slice(2), 10,
>> IPVK_IndirectCallTarget,
>> +                    5);
>>    Res = getValueProfDataFromInst(*Inst, IPVK_IndirectCallTarget, 5,
>>                                        ValueData, N, T);
>>    ASSERT_TRUE(Res);
>> Index: lib/ProfileData/InstrProf.cpp
>> ===================================================================
>> --- lib/ProfileData/InstrProf.cpp
>> +++ lib/ProfileData/InstrProf.cpp
>> @@ -606,11 +606,12 @@
>>    std::unique_ptr<InstrProfValueData[]> VD =
>>        InstrProfR.getValueForSite(ValueKind, SiteIdx, &Sum);
>>
>> -  annotateValueSite(M, Inst, VD.get(), NV, Sum, ValueKind, MaxMDCount);
>> +  ArrayRef<InstrProfValueData> VDArrayRef(VD.get(), NV);
>> +  annotateValueSite(M, Inst, VDArrayRef, Sum, ValueKind, MaxMDCount);
>>  }
>>
>>  void annotateValueSite(Module &M, Instruction &Inst,
>> -                       const InstrProfValueData VD[], uint32_t NV,
>> +                       const ArrayRef<InstrProfValueData> VDArrayRef,
>>
>
> Const on a value parameter is pretty non-idiomatic. Perhaps drop the const?
>
>
>>                         uint64_t Sum, InstrProfValueKind ValueKind,
>>                         uint32_t MaxMDCount) {
>>    LLVMContext &Ctx = M.getContext();
>> @@ -627,11 +628,11 @@
>>
>>    // Value Profile Data
>>    uint32_t MDCount = MaxMDCount;
>> -  for (uint32_t I = 0; I < NV; ++I) {
>> +  for (auto &I : VDArrayRef) {
>>      Vals.push_back(MDHelper.createConstant(
>> -        ConstantInt::get(Type::getInt64Ty(Ctx), VD[I].Value)));
>> +        ConstantInt::get(Type::getInt64Ty(Ctx), I.Value)));
>>      Vals.push_back(MDHelper.createConstant(
>> -        ConstantInt::get(Type::getInt64Ty(Ctx), VD[I].Count)));
>> +        ConstantInt::get(Type::getInt64Ty(Ctx), I.Count)));
>>      if (--MDCount == 0)
>>        break;
>>    }
>> Index: include/llvm/ProfileData/InstrProf.h
>> ===================================================================
>> --- include/llvm/ProfileData/InstrProf.h
>> +++ include/llvm/ProfileData/InstrProf.h
>> @@ -229,10 +229,9 @@
>>                         const InstrProfRecord &InstrProfR,
>>                         InstrProfValueKind ValueKind, uint32_t SiteIndx,
>>                         uint32_t MaxMDCount = 3);
>> -/// Same as the above interface but using the ValueData array directly,
>> as
>> -/// well as \p Sum.
>> +/// Same as the above interface but using an ArrayRef, as well as \p Sum.
>>  void annotateValueSite(Module &M, Instruction &Inst,
>> -                       const InstrProfValueData VD[], uint32_t NV,
>> +                       const ArrayRef<InstrProfValueData> VDArrayRef,
>>                         uint64_t Sum, InstrProfValueKind ValueKind,
>>                         uint32_t MaxMDCount);
>>
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160330/17b7d273/attachment.html>