[llvm] r264641 - Reapply "[PGO] Fix name encoding for ObjC-like functions"
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 28 13:57:53 PDT 2016
On Mon, Mar 28, 2016 at 1:52 PM, Vedant Kumar <vsk at apple.com> wrote:
>
> > On Mar 28, 2016, at 1:48 PM, Xinliang David Li <xinliangli at gmail.com>
> wrote:
> >
> > I missed this in the original review.
> >
> > In theory, changes like this requires bumping up the raw and coverage
> format version. However since it is relatively new, I think it would be ok
> to not bumping up the version and treat this like a bug fix.
>
> I'm inclined to agree with this. We didn't ship this in 3.8, so it seems
> like there's room to treat this as a bug fix.
>
>
> > Also why does the expected result change in
> test/tools/llvm-cov/Inputs/instrprof-comdat.h need to be bundled with this
> patch?
>
> The file is compiled with a new clang in order to pick up this change.
>
> But, new clangs contain a bug fix which gets the correct coverage for the
> `Foo::DoIt`.
>
> I bundled the changes together to avoid weakening the test case with this
> patch.
>
> I can split it up into two changes if you prefer.
>
Splitting it will help tracking the motivation of the changes better, so I
think it is more peferred.
David
>
> vedant
>
>
> > On Mon, Mar 28, 2016 at 1:12 PM, Vedant Kumar via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> > Author: vedantk
> > Date: Mon Mar 28 15:12:07 2016
> > New Revision: 264641
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=264641&view=rev
> > Log:
> > Reapply "[PGO] Fix name encoding for ObjC-like functions"
> >
> > Function names in ObjC can have spaces in them. This interacts poorly
> > with name compression, which uses spaces to separate PGO names. Fix the
> > issue by using a different separator and update a test.
> >
> > I chose "\01" as the separator because 1) it's non-printable, 2) we
> > strip it from PGO names, and 3) it's the next natural choice once "\00"
> > is discarded (that one's overloaded).
> >
> > This reverts the revert commit beaf3d18. What's changed?
> >
> > - I fixed up the covmap-V2 binary format tests using a linux VM.
> > - I updated the expected counts in instrprof-comdat.h to account for
> > the fact that there have been bugfixes to clang coverage.
> > - I added an assert to make sure we don't get bitten by this again.
> >
> > Differential Revision: http://reviews.llvm.org/D18516
> >
> > Modified:
> > llvm/trunk/include/llvm/ProfileData/InstrProf.h
> > llvm/trunk/lib/ProfileData/InstrProf.cpp
> > llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux32l
> > llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux64l
> > llvm/trunk/test/tools/llvm-cov/Inputs/elf_binary_comdat.profdata
> > llvm/trunk/test/tools/llvm-cov/Inputs/instrprof-comdat.h
> > llvm/trunk/test/tools/llvm-profdata/Inputs/c-general.profraw
> > llvm/trunk/test/tools/llvm-profdata/raw-32-bits-be.test
> > llvm/trunk/test/tools/llvm-profdata/raw-32-bits-le.test
> > llvm/trunk/test/tools/llvm-profdata/raw-64-bits-be.test
> > llvm/trunk/test/tools/llvm-profdata/raw-64-bits-le.test
> > llvm/trunk/unittests/ProfileData/InstrProfTest.cpp
> >
> > Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=264641&r1=264640&r2=264641&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
> > +++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Mon Mar 28 15:12:07
> 2016
> > @@ -151,6 +151,9 @@ inline StringRef getInstrProfFileOverrid
> > return "__llvm_profile_override_default_filename";
> > }
> >
> > +/// Return the marker used to separate PGO names during serialization.
> > +inline StringRef getInstrProfNameSeparator() { return "\01"; }
> > +
> > /// Return the modified name for function \c F suitable to be
> > /// used the key for profile lookup.
> > std::string getPGOFuncName(const Function &F,
> >
> > Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=264641&r1=264640&r2=264641&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)
> > +++ llvm/trunk/lib/ProfileData/InstrProf.cpp Mon Mar 28 15:12:07 2016
> > @@ -158,9 +158,15 @@ void InstrProfSymtab::create(const Modul
> >
> > int collectPGOFuncNameStrings(const std::vector<std::string> &NameStrs,
> > bool doCompression, std::string &Result) {
> > + assert(NameStrs.size() && "No name data to emit");
> > +
> > uint8_t Header[16], *P = Header;
> > std::string UncompressedNameStrings =
> > - join(NameStrs.begin(), NameStrs.end(), StringRef(" "));
> > + join(NameStrs.begin(), NameStrs.end(),
> getInstrProfNameSeparator());
> > +
> > + assert(StringRef(UncompressedNameStrings)
> > + .count(getInstrProfNameSeparator()) ==
> (NameStrs.size() - 1) &&
> > + "PGO name is invalid (contains separator token)");
> >
> > unsigned EncLen = encodeULEB128(UncompressedNameStrings.length(), P);
> > P += EncLen;
> > @@ -238,7 +244,7 @@ int readPGOFuncNameStrings(StringRef Nam
> > }
> > // Now parse the name strings.
> > SmallVector<StringRef, 0> Names;
> > - NameStrings.split(Names, ' ');
> > + NameStrings.split(Names, getInstrProfNameSeparator());
> > for (StringRef &Name : Names)
> > Symtab.addFuncName(Name);
> >
> >
> > Modified:
> llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux32l
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux32l?rev=264641&r1=264640&r2=264641&view=diff
> >
> ==============================================================================
> > Binary files - no diff available.
> >
> > Modified:
> llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux64l
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-cov/Inputs/binary-formats.v2.linux64l?rev=264641&r1=264640&r2=264641&view=diff
> >
> ==============================================================================
> > Binary files - no diff available.
> >
> > Modified:
> llvm/trunk/test/tools/llvm-cov/Inputs/elf_binary_comdat.profdata
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-cov/Inputs/elf_binary_comdat.profdata?rev=264641&r1=264640&r2=264641&view=diff
> >
> ==============================================================================
> > Binary files - no diff available.
> >
> > Modified: llvm/trunk/test/tools/llvm-cov/Inputs/instrprof-comdat.h
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-cov/Inputs/instrprof-comdat.h?rev=264641&r1=264640&r2=264641&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/test/tools/llvm-cov/Inputs/instrprof-comdat.h (original)
> > +++ llvm/trunk/test/tools/llvm-cov/Inputs/instrprof-comdat.h Mon Mar 28
> 15:12:07 2016
> > @@ -13,7 +13,12 @@ template <class T> T FOO<T>::DoIt(T ti)
> > t += I; // HEADER: 20| [[@LINE]]|
> t += I;
> > if (I > ti / 2) // HEADER: 20| [[@LINE]]|
> if (I > ti
> > t -= 1; // HEADER: 8| [[@LINE]]|
> t -= 1;
> > - } // HEADER: 10| [[@LINE]]| }
> > - // HEADER: 1| [[@LINE]]|
> > - return t; // HEADER: 1| [[@LINE]]|
> return t;
> > + } // HEADER: 20| [[@LINE]]| }
> > + // HEADER: 2| [[@LINE]]|
> > + return t; // HEADER: 2| [[@LINE]]|
> return t;
> > }
> > +
> > +// To generate the binaries which correspond to this file, you must
> first
> > +// compile a program with two calls to Foo<int>::DoIt(10) for each
> desired
> > +// architecture. Collect a raw profile from any one of these binaries,
> index
> > +// it, and check it in along with the executables.
> >
> > Modified: llvm/trunk/test/tools/llvm-profdata/Inputs/c-general.profraw
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/Inputs/c-general.profraw?rev=264641&r1=264640&r2=264641&view=diff
> >
> ==============================================================================
> > Binary files
> llvm/trunk/test/tools/llvm-profdata/Inputs/c-general.profraw (original) and
> llvm/trunk/test/tools/llvm-profdata/Inputs/c-general.profraw Mon Mar 28
> 15:12:07 2016 differ
> >
> > Modified: llvm/trunk/test/tools/llvm-profdata/raw-32-bits-be.test
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/raw-32-bits-be.test?rev=264641&r1=264640&r2=264641&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/test/tools/llvm-profdata/raw-32-bits-be.test (original)
> > +++ llvm/trunk/test/tools/llvm-profdata/raw-32-bits-be.test Mon Mar 28
> 15:12:07 2016
> > @@ -28,7 +28,7 @@ RUN: printf '\0\0\0\0\0\0\0\0' >> %t
> > RUN: printf '\0\0\0\0\0\0\0\023' >> %t
> > RUN: printf '\0\0\0\0\0\0\0\067' >> %t
> > RUN: printf '\0\0\0\0\0\0\0\101' >> %t
> > -RUN: printf '\7\0foo bar\0\0\0\0\0\0\0' >> %t
> > +RUN: printf '\7\0foo\1bar\0\0\0\0\0\0\0' >> %t
> >
> > RUN: llvm-profdata show %t -all-functions -counts | FileCheck %s
> >
> >
> > Modified: llvm/trunk/test/tools/llvm-profdata/raw-32-bits-le.test
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/raw-32-bits-le.test?rev=264641&r1=264640&r2=264641&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/test/tools/llvm-profdata/raw-32-bits-le.test (original)
> > +++ llvm/trunk/test/tools/llvm-profdata/raw-32-bits-le.test Mon Mar 28
> 15:12:07 2016
> > @@ -28,7 +28,7 @@ RUN: printf '\0\0\0\0\0\0\0\0' >> %t
> > RUN: printf '\023\0\0\0\0\0\0\0' >> %t
> > RUN: printf '\067\0\0\0\0\0\0\0' >> %t
> > RUN: printf '\101\0\0\0\0\0\0\0' >> %t
> > -RUN: printf '\7\0foo bar\0\0\0\0\0\0\0' >> %t
> > +RUN: printf '\7\0foo\1bar\0\0\0\0\0\0\0' >> %t
> >
> > RUN: llvm-profdata show %t -all-functions -counts | FileCheck %s
> >
> >
> > Modified: llvm/trunk/test/tools/llvm-profdata/raw-64-bits-be.test
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/raw-64-bits-be.test?rev=264641&r1=264640&r2=264641&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/test/tools/llvm-profdata/raw-64-bits-be.test (original)
> > +++ llvm/trunk/test/tools/llvm-profdata/raw-64-bits-be.test Mon Mar 28
> 15:12:07 2016
> > @@ -26,7 +26,7 @@ RUN: printf '\0\0\0\02\0\0\0\0' >> %t
> > RUN: printf '\0\0\0\0\0\0\0\023' >> %t
> > RUN: printf '\0\0\0\0\0\0\0\067' >> %t
> > RUN: printf '\0\0\0\0\0\0\0\101' >> %t
> > -RUN: printf '\7\0foo bar\0\0\0\0\0\0\0' >> %t
> > +RUN: printf '\7\0foo\1bar\0\0\0\0\0\0\0' >> %t
> >
> > RUN: llvm-profdata show %t -all-functions -counts | FileCheck %s
> >
> >
> > Modified: llvm/trunk/test/tools/llvm-profdata/raw-64-bits-le.test
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-profdata/raw-64-bits-le.test?rev=264641&r1=264640&r2=264641&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/test/tools/llvm-profdata/raw-64-bits-le.test (original)
> > +++ llvm/trunk/test/tools/llvm-profdata/raw-64-bits-le.test Mon Mar 28
> 15:12:07 2016
> > @@ -26,7 +26,7 @@ RUN: printf '\02\0\0\0\0\0\0\0' >> %t
> > RUN: printf '\023\0\0\0\0\0\0\0' >> %t
> > RUN: printf '\067\0\0\0\0\0\0\0' >> %t
> > RUN: printf '\101\0\0\0\0\0\0\0' >> %t
> > -RUN: printf '\7\0foo bar\0\0\0\0\0\0\0' >> %t
> > +RUN: printf '\7\0foo\1bar\0\0\0\0\0\0\0' >> %t
> >
> > RUN: llvm-profdata show %t -all-functions -counts | FileCheck %s
> >
> >
> > Modified: llvm/trunk/unittests/ProfileData/InstrProfTest.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/InstrProfTest.cpp?rev=264641&r1=264640&r2=264641&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/unittests/ProfileData/InstrProfTest.cpp (original)
> > +++ llvm/trunk/unittests/ProfileData/InstrProfTest.cpp Mon Mar 28
> 15:12:07 2016
> > @@ -893,7 +893,7 @@ TEST_P(MaybeSparseInstrProfTest, instr_p
> > OS << "func_" << I;
> > FuncNames1.push_back(OS.str());
> > str.clear();
> > - OS << "fooooooooooooooo_" << I;
> > + OS << "f oooooooooooooo_" << I;
> > FuncNames1.push_back(OS.str());
> > str.clear();
> > OS << "BAR_" << I;
> > @@ -931,7 +931,7 @@ TEST_P(MaybeSparseInstrProfTest, instr_p
> > StringRef R =
> Symtab.getFuncName(IndexedInstrProf::ComputeHash(FuncNames1[0]));
> > ASSERT_EQ(StringRef("func_0"), R);
> > R =
> Symtab.getFuncName(IndexedInstrProf::ComputeHash(FuncNames1[1]));
> > - ASSERT_EQ(StringRef("fooooooooooooooo_0"), R);
> > + ASSERT_EQ(StringRef("f oooooooooooooo_0"), R);
> > for (int I = 0; I < 3; I++) {
> > std::string N[4];
> > N[0] = FuncNames1[2 * I];
> >
> >
> > _______________________________________________
> > 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/20160328/bb4e5e3a/attachment.html>
More information about the llvm-commits
mailing list