[llvm] r328132 - [InstrProf] Support for external functions in text format.
Benjamin Kramer via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 22 08:34:41 PDT 2018
This broke our internal FDO bootstrap. I reverted the change for now
in r328207, let me know if you need more information on reproducing
the issue.
On Wed, Mar 21, 2018 at 8:06 PM, Mircea Trofin via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
> 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&r2=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=328131&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=328132&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=328131&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
More information about the llvm-commits
mailing list