[llvm] r328132 - [InstrProf] Support for external functions in text format.
Mircea Trofin via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 21 14:59:15 PDT 2018
Oh! Thanks!
On Wed, Mar 21, 2018 at 2:55 PM Reid Kleckner <rnk at google.com> wrote:
> I went ahead and fixed this in r328157. In the future, avoid joining
> /dev/null into arguments.
>
>
> On Wed, Mar 21, 2018 at 2:34 PM via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Hi Mircea,
>>
>> Your change added a test which is failing because /dev/null does not
>> exist on Windows. Can you fix the test?
>>
>>
>> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/16048
>>
>> $ "not"
>> "C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.obj\bin\llvm-profdata.EXE"
>> "merge"
>> "C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.obj\test\tools\llvm-profdata\Output\invalid-profdata.test.tmp.input"
>> "-text" "-output=/dev/null"
>> $
>> "C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.obj\bin\FileCheck.EXE"
>> "C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\test\tools\llvm-profdata\invalid-profdata.test"
>> "--check-prefix=BROKEN"
>> # command stderr:
>> C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\test\tools\llvm-profdata\invalid-profdata.test:24:9:
>> error: expected string not found in input
>>
>> BROKEN: Malformed instrumentation profile data
>>
>> ^
>>
>> <stdin>:1:1: note: scanning from here
>>
>> error: /dev/null: no such file or directory
>>
>> ^
>>
>> <stdin>:1:13: note: possible intended match here
>>
>> error: /dev/null: no such file or directory
>>
>> ^
>>
>>
>> error: command failed with exit status: 1
>>
>>
>> Douglas Yung
>>
>> > -----Original Message-----
>> > From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On
>> Behalf Of
>> > Mircea Trofin via llvm-commits
>> > Sent: Wednesday, March 21, 2018 12:06
>> > To: llvm-commits at lists.llvm.org
>> > Subject: [llvm] r328132 - [InstrProf] Support for external functions in
>> text
>> > format.
>> >
>> > Author: mtrofin
>> > Date: Wed Mar 21 12:06:06 2018
>> > New Revision: 328132
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=328132&view=rev
>> > Log:
>> > [InstrProf] Support for external functions in text format.
>> >
>> > Summary:
>> > External functions appearing as indirect call targets could not be
>> found in
>> > the SymTab, and the value:counter record was represented, in the text
>> format,
>> > using an empty string for the name. This would then cause a silent
>> parsing
>> > error when reading.
>> >
>> > This CL:
>> > - adds explicit support for such functions
>> > - fixes the places where we would not propagate errors when reading
>> > - addresses a performance issue due to eager resorting of the SymTab.
>> >
>> > Reviewers: xur, eraman, davidxl
>> >
>> > Reviewed By: davidxl
>> >
>> > Subscribers: llvm-commits
>> >
>> > Differential Revision: https://reviews.llvm.org/D44717
>> >
>> > Added:
>> > llvm/trunk/test/tools/llvm-profdata/invalid-profdata.test
>> > Modified:
>> > llvm/trunk/include/llvm/ProfileData/InstrProf.h
>> > llvm/trunk/include/llvm/ProfileData/InstrProfReader.h
>> > llvm/trunk/lib/ProfileData/InstrProf.cpp
>> > llvm/trunk/lib/ProfileData/InstrProfReader.cpp
>> > llvm/trunk/lib/ProfileData/InstrProfWriter.cpp
>> > 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=328132&r1=328131&r
>> > 2=328132&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
>> > +++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Wed Mar 21 12:06:06
>> > +++ 2018
>> > @@ -425,9 +425,20 @@ private:
>> > // A map from function runtime address to function name MD5 hash.
>> > // This map is only populated and used by raw instr profile reader.
>> > AddrHashMap AddrToMD5Map;
>> > + bool Sorted = false;
>> > +
>> > + static StringRef getExternalSymbol() {
>> > + return "** External Symbol **";
>> > + }
>> > +
>> > + // If the symtab is created by a series of calls to \c addFuncName,
>> > + \c // finalizeSymtab needs to be called before looking up function
>> names.
>> > + // This is required because the underlying map is a vector (for space
>> > + // efficiency) which needs to be sorted.
>> > + inline void finalizeSymtab();
>> >
>> > public:
>> > - InstrProfSymtab() = default;
>> > + InstrProfSymtab() = default;
>> >
>> > /// Create InstrProfSymtab from an object file section which
>> > /// contains function PGO names. When section may contain raw @@
>> -456,21
>> > +467,17 @@ public:
>> > /// \p IterRange. This interface is used by IndexedProfReader.
>> > template <typename NameIterRange> Error create(const NameIterRange
>> > &IterRange);
>> >
>> > - // If the symtab is created by a series of calls to \c addFuncName,
>> \c
>> > - // finalizeSymtab needs to be called before looking up function
>> names.
>> > - // This is required because the underlying map is a vector (for space
>> > - // efficiency) which needs to be sorted.
>> > - inline void finalizeSymtab();
>> > -
>> > /// Update the symtab by adding \p FuncName to the table. This
>> interface
>> > /// is used by the raw and text profile readers.
>> > Error addFuncName(StringRef FuncName) {
>> > if (FuncName.empty())
>> > return make_error<InstrProfError>(instrprof_error::malformed);
>> > auto Ins = NameTab.insert(FuncName);
>> > - if (Ins.second)
>> > + if (Ins.second) {
>> > MD5NameMap.push_back(std::make_pair(
>> > IndexedInstrProf::ComputeHash(FuncName),
>> Ins.first->getKey()));
>> > + Sorted = false;
>> > + }
>> > return Error::success();
>> > }
>> >
>> > @@ -491,6 +498,16 @@ public:
>> > /// If not found, return an empty string.
>> > inline StringRef getFuncName(uint64_t FuncMD5Hash);
>> >
>> > + /// Just like getFuncName, except that it will return a non-empty
>> > + StringRef /// if the function is external to this symbol table. All
>> > + such cases /// will be represented using the same StringRef value.
>> > + inline StringRef getFuncNameOrExternalSymbol(uint64_t FuncMD5Hash);
>> > +
>> > + /// True if Symbol is the value used to represent external symbols.
>> > + static bool isExternalSymbol(const StringRef &Symbol) {
>> > + return Symbol == InstrProfSymtab::getExternalSymbol();
>> > + }
>> > +
>> > /// Return function from the name's md5 hash. Return nullptr if not
>> found.
>> > inline Function *getFunction(uint64_t FuncMD5Hash);
>> >
>> > @@ -524,14 +541,25 @@ Error InstrProfSymtab::create(const Name }
>> >
>> > void InstrProfSymtab::finalizeSymtab() {
>> > + if (Sorted)
>> > + return;
>> > std::sort(MD5NameMap.begin(), MD5NameMap.end(), less_first());
>> > std::sort(MD5FuncMap.begin(), MD5FuncMap.end(), less_first());
>> > std::sort(AddrToMD5Map.begin(), AddrToMD5Map.end(), less_first());
>> > AddrToMD5Map.erase(std::unique(AddrToMD5Map.begin(),
>> AddrToMD5Map.end()),
>> > AddrToMD5Map.end());
>> > + Sorted = true;
>> > +}
>> > +
>> > +StringRef InstrProfSymtab::getFuncNameOrExternalSymbol(uint64_t
>> > +FuncMD5Hash) {
>> > + StringRef ret = getFuncName(FuncMD5Hash);
>> > + if (ret.empty())
>> > + return InstrProfSymtab::getExternalSymbol();
>> > + return ret;
>> > }
>> >
>> > StringRef InstrProfSymtab::getFuncName(uint64_t FuncMD5Hash) {
>> > + finalizeSymtab();
>> > auto Result =
>> > std::lower_bound(MD5NameMap.begin(), MD5NameMap.end(),
>> FuncMD5Hash,
>> > [](const std::pair<uint64_t, std::string> &LHS,
>> @@ -
>> > 542,6 +570,7 @@ StringRef InstrProfSymtab::getFuncName(u }
>> >
>> > Function* InstrProfSymtab::getFunction(uint64_t FuncMD5Hash) {
>> > + finalizeSymtab();
>> > auto Result =
>> > std::lower_bound(MD5FuncMap.begin(), MD5FuncMap.end(),
>> FuncMD5Hash,
>> > [](const std::pair<uint64_t, Function*> &LHS,
>> >
>> > Modified: llvm/trunk/include/llvm/ProfileData/InstrProfReader.h
>> > URL: http://llvm.org/viewvc/llvm-
>> >
>> project/llvm/trunk/include/llvm/ProfileData/InstrProfReader.h?rev=328132&r1=32
>> > 8131&r2=328132&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/include/llvm/ProfileData/InstrProfReader.h (original)
>> > +++ llvm/trunk/include/llvm/ProfileData/InstrProfReader.h Wed Mar 21
>> > +++ 12:06:06 2018
>> > @@ -101,7 +101,7 @@ protected:
>> > return make_error<InstrProfError>(Err);
>> > }
>> >
>> > - Error error(Error E) { return
>> error(InstrProfError::take(std::move(E))); }
>> > + Error error(Error &&E) { return
>> > + error(InstrProfError::take(std::move(E))); }
>> >
>> > /// Clear the current error and return a successful one.
>> > Error success() { return error(instrprof_error::success); }
>> >
>> > Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp
>> > URL: http://llvm.org/viewvc/llvm-
>> >
>> project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=328132&r1=328131&r2=32813
>> > 2&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)
>> > +++ llvm/trunk/lib/ProfileData/InstrProf.cpp Wed Mar 21 12:06:06 2018
>> > @@ -355,7 +355,7 @@ Error InstrProfSymtab::create(Module &M,
>> > }
>> > }
>> > }
>> > -
>> > + Sorted = false;
>> > finalizeSymtab();
>> > return Error::success();
>> > }
>> > @@ -461,7 +461,6 @@ Error readPGOFuncNameStrings(StringRef N
>> > while (P < EndP && *P == 0)
>> > P++;
>> > }
>> > - Symtab.finalizeSymtab();
>> > return Error::success();
>> > }
>> >
>> >
>> > Modified: llvm/trunk/lib/ProfileData/InstrProfReader.cpp
>> > URL: http://llvm.org/viewvc/llvm-
>> >
>> project/llvm/trunk/lib/ProfileData/InstrProfReader.cpp?rev=328132&r1=328131&r2
>> > =328132&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/lib/ProfileData/InstrProfReader.cpp (original)
>> > +++ llvm/trunk/lib/ProfileData/InstrProfReader.cpp Wed Mar 21 12:06:06
>> > +++ 2018
>> > @@ -200,9 +200,13 @@ TextInstrProfReader::readValueProfileDat
>> > std::pair<StringRef, StringRef> VD = Line->rsplit(':');
>> > uint64_t TakenCount, Value;
>> > if (ValueKind == IPVK_IndirectCallTarget) {
>> > - if (Error E = Symtab->addFuncName(VD.first))
>> > - return E;
>> > - Value = IndexedInstrProf::ComputeHash(VD.first);
>> > + if (InstrProfSymtab::isExternalSymbol(VD.first)) {
>> > + Value = 0;
>> > + } else {
>> > + if (Error E = Symtab->addFuncName(VD.first))
>> > + return E;
>> > + Value = IndexedInstrProf::ComputeHash(VD.first);
>> > + }
>> > } else {
>> > READ_NUM(VD.first, Value);
>> > }
>> > @@ -227,14 +231,13 @@ Error TextInstrProfReader::readNextRecor
>> > ++Line;
>> > // If we hit EOF while looking for a name, we're done.
>> > if (Line.is_at_end()) {
>> > - Symtab->finalizeSymtab();
>> > return error(instrprof_error::eof);
>> > }
>> >
>> > // Read the function name.
>> > Record.Name = *Line++;
>> > if (Error E = Symtab->addFuncName(Record.Name))
>> > - return E;
>> > + return error(std::move(E));
>> >
>> > // Read the function hash.
>> > if (Line.is_at_end())
>> > @@ -265,11 +268,8 @@ Error TextInstrProfReader::readNextRecor
>> >
>> > // Check if value profile data exists and read it if so.
>> > if (Error E = readValueProfileData(Record))
>> > - return E;
>> > + return error(std::move(E));
>> >
>> > - // This is needed to avoid two pass parsing because llvm-profdata
>> > - // does dumping while reading.
>> > - Symtab->finalizeSymtab();
>> > return success();
>> > }
>> >
>> > @@ -331,7 +331,6 @@ Error RawInstrProfReader<IntPtrT>::creat
>> > continue;
>> > Symtab.mapAddress(FPtr, I->NameRef);
>> > }
>> > - Symtab.finalizeSymtab();
>> > return success();
>> > }
>> >
>> > @@ -449,23 +448,23 @@ Error RawInstrProfReader<IntPtrT>::readN
>> > if (atEnd())
>> > // At this point, ValueDataStart field points to the next header.
>> > if (Error E = readNextHeader(getNextHeaderPos()))
>> > - return E;
>> > + return error(std::move(E));
>> >
>> > // Read name ad set it in Record.
>> > if (Error E = readName(Record))
>> > - return E;
>> > + return error(std::move(E));
>> >
>> > // Read FuncHash and set it in Record.
>> > if (Error E = readFuncHash(Record))
>> > - return E;
>> > + return error(std::move(E));
>> >
>> > // Read raw counts and set Record.
>> > if (Error E = readRawCounts(Record))
>> > - return E;
>> > + return error(std::move(E));
>> >
>> > // Read value data and set Record.
>> > if (Error E = readValueProfilingData(Record))
>> > - return E;
>> > + return error(std::move(E));
>> >
>> > // Iterate.
>> > advanceData();
>> >
>> > Modified: llvm/trunk/lib/ProfileData/InstrProfWriter.cpp
>> > URL: http://llvm.org/viewvc/llvm-
>> >
>> project/llvm/trunk/lib/ProfileData/InstrProfWriter.cpp?rev=328132&r1=328131&r2
>> > =328132&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/lib/ProfileData/InstrProfWriter.cpp (original)
>> > +++ llvm/trunk/lib/ProfileData/InstrProfWriter.cpp Wed Mar 21 12:06:06
>> > +++ 2018
>> > @@ -361,7 +361,8 @@ void InstrProfWriter::writeRecordInText(
>> > std::unique_ptr<InstrProfValueData[]> VD =
>> Func.getValueForSite(VK, S);
>> > for (uint32_t I = 0; I < ND; I++) {
>> > if (VK == IPVK_IndirectCallTarget)
>> > - OS << Symtab.getFuncName(VD[I].Value) << ":" << VD[I].Count
>> <<
>> > "\n";
>> > + OS << Symtab.getFuncNameOrExternalSymbol(VD[I].Value) << ":"
>> > + << VD[I].Count << "\n";
>> > else
>> > OS << VD[I].Value << ":" << VD[I].Count << "\n";
>> > }
>> > @@ -379,7 +380,6 @@ Error InstrProfWriter::writeText(raw_fd_
>> > if (shouldEncodeData(I.getValue()))
>> > if (Error E = Symtab.addFuncName(I.getKey()))
>> > return E;
>> > - Symtab.finalizeSymtab();
>> >
>> > for (const auto &I : FunctionData)
>> > if (shouldEncodeData(I.getValue()))
>> >
>> > Added: llvm/trunk/test/tools/llvm-profdata/invalid-profdata.test
>> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-
>> > profdata/invalid-profdata.test?rev=328132&view=auto
>> >
>> ==============================================================================
>> > --- llvm/trunk/test/tools/llvm-profdata/invalid-profdata.test (added)
>> > +++ llvm/trunk/test/tools/llvm-profdata/invalid-profdata.test Wed Mar 21
>> > +++ 12:06:06 2018
>> > @@ -0,0 +1,50 @@
>> > +RUN: echo ":ir" > %t.input
>> > +RUN: echo "_ZN6Thread5StartEv" >> %t.input
>> > +RUN: echo "# Func Hash:" >> %t.input
>> > +RUN: echo "288793635542036872" >> %t.input
>> > +RUN: echo "# Num Counters:" >> %t.input
>> > +RUN: echo "3" >> %t.input
>> > +RUN: echo "# Counter Values:" >> %t.input
>> > +RUN: echo "0" >> %t.input
>> > +RUN: echo "12" >> %t.input
>> > +RUN: echo "12" >> %t.input
>> > +RUN: echo "# Num Value Kinds:" >> %t.input
>> > +RUN: echo "1" >> %t.input
>> > +RUN: echo "# ValueKind = IPVK_IndirectCallTarget:" >> %t.input
>> > +RUN: echo "0" >> %t.input
>> > +RUN: echo "# NumValueSites:" >> %t.input
>> > +RUN: echo "2" >> %t.input
>> > +RUN: echo "2" >> %t.input
>> > +RUN: echo "f1:10" >> %t.input
>> > +RUN: echo "f2:0" >> %t.input
>> > +RUN: echo "1" >> %t.input
>> > +RUN: echo ":10" >> %t.input
>> > +
>> > +RUN: not llvm-profdata merge %t.input -text -output=/dev/null 2>&1 |
>> > +FileCheck %s --check-prefix=BROKEN
>> > +BROKEN: Malformed instrumentation profile data
>> > +
>> > +RUN: echo ":ir" > %t.input
>> > +RUN: echo "_ZN6Thread5StartEv" >> %t.input
>> > +RUN: echo "# Func Hash:" >> %t.input
>> > +RUN: echo "288793635542036872" >> %t.input
>> > +RUN: echo "# Num Counters:" >> %t.input
>> > +RUN: echo "3" >> %t.input
>> > +RUN: echo "# Counter Values:" >> %t.input
>> > +RUN: echo "0" >> %t.input
>> > +RUN: echo "12" >> %t.input
>> > +RUN: echo "12" >> %t.input
>> > +RUN: echo "# Num Value Kinds:" >> %t.input
>> > +RUN: echo "1" >> %t.input
>> > +RUN: echo "# ValueKind = IPVK_IndirectCallTarget:" >> %t.input
>> > +RUN: echo "0" >> %t.input
>> > +RUN: echo "# NumValueSites:" >> %t.input
>> > +RUN: echo "2" >> %t.input
>> > +RUN: echo "2" >> %t.input
>> > +RUN: echo "f1:10" >> %t.input
>> > +RUN: echo "f2:0" >> %t.input
>> > +RUN: echo "1" >> %t.input
>> > +RUN: echo "** External Symbol **:10" >> %t.input
>> > +
>> > +# RUN: llvm-profdata merge %t.input -text -output=%t.out && cat %t.out
>> > +| FileCheck %s
>> > +
>> > +CHECK: ** External Symbol **:10
>> >
>> > Modified: llvm/trunk/unittests/ProfileData/InstrProfTest.cpp
>> > URL: http://llvm.org/viewvc/llvm-
>> >
>> project/llvm/trunk/unittests/ProfileData/InstrProfTest.cpp?rev=328132&r1=32813
>> > 1&r2=328132&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/unittests/ProfileData/InstrProfTest.cpp (original)
>> > +++ llvm/trunk/unittests/ProfileData/InstrProfTest.cpp Wed Mar 21
>> > +++ 12:06:06 2018
>> > @@ -769,7 +769,6 @@ TEST_P(MaybeSparseInstrProfTest, value_p
>> > Symtab.mapAddress(uint64_t(callee3), 0x3000ULL);
>> > Symtab.mapAddress(uint64_t(callee4), 0x4000ULL);
>> > // Missing mapping for callee5
>> > - Symtab.finalizeSymtab();
>> >
>> > VPData->deserializeTo(Record, &Symtab.getAddrHashMap());
>> >
>> > @@ -858,8 +857,6 @@ TEST_P(MaybeSparseInstrProfTest, instr_p
>> > EXPECT_THAT_ERROR(Symtab.addFuncName("blah_1"), Succeeded());
>> > EXPECT_THAT_ERROR(Symtab.addFuncName("blah_2"), Succeeded());
>> > EXPECT_THAT_ERROR(Symtab.addFuncName("blah_3"), Succeeded());
>> > - // Finalize it
>> > - Symtab.finalizeSymtab();
>> >
>> > // Check again
>> > R = Symtab.getFuncName(IndexedInstrProf::ComputeHash("blah_1"));
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> _______________________________________________
>> 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/20180321/fe52d0e3/attachment.html>
More information about the llvm-commits
mailing list