[llvm] r344184 - Support for remapping profile data when symbols change, for
via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 10 19:09:29 PDT 2018
Hi Richard,
Your change is causing a build failure on the PS4 Windows bot. Can you take a look?
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/20584
FAILED: lib/ProfileData/CMakeFiles/LLVMProfileData.dir/InstrProfReader.cpp.obj
C:\PROGRA~2\MICROS~1.0\VC\bin\cl.exe /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_FILE_OFFSET_BITS=64 -D_HAS_EXCEPTIONS=0 -D_LARGEFILE_SOURCE -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib\ProfileData -IC:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\lib\ProfileData -Iinclude -IC:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\include /DWIN32 /D_WINDOWS /Zc:inline /Zc:strictStrings /Oi /Zc:rvalueCast /W4 -wd4141 -wd4146 -wd4180 -wd4244 -wd4258 -wd4267 -wd4291 -wd4345 -wd4351 -wd4355 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4800 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4324 -w14062 -we4238 /MD /O2 /Ob2 -UNDEBUG /EHs-c- /GR- /showIncludes /Folib\ProfileData\CMakeFiles\LLVMProfileData.dir\InstrProfReader.cpp.obj /Fdlib\ProfileData\CMakeFiles\LLVMProfileData.dir\LLVMProfileData.pdb /FS -c C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\lib\ProfileData\InstrProfReader.cpp
C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\include\llvm/ADT/DenseMap.h(644): error C2872: 'detail': ambiguous symbol
C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\include\llvm/ADT/DenseMap.h(35): note: could be 'llvm::detail'
C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\include\llvm/Support/Endian.h(205): note: or 'llvm::support::detail'
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\type_traits(348): note: see reference to class template instantiation 'llvm::InstrProfReaderItaniumRemapper<llvm::OnDiskHashTableImplV3>' being compiled
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\memory(1185): note: see reference to class template instantiation 'std::is_convertible<_Ty2 *,_Ty *>' being compiled
with
[
_Ty2=llvm::InstrProfReaderItaniumRemapper<llvm::OnDiskHashTableImplV3>,
_Ty=llvm::InstrProfReaderRemapper
]
C:\ps4-buildslave2\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\lib\ProfileData\InstrProfReader.cpp(828): note: see reference to class template instantiation 'std::is_assignable<_Dx &,std::default_delete<_Ty> &&>' being compiled
with
[
_Dx=std::default_delete<llvm::InstrProfReaderRemapper>,
_Ty=llvm::InstrProfReaderItaniumRemapper<llvm::OnDiskHashTableImplV3>
]
Douglas Yung
> -----Original Message-----
> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On
> Behalf Of Richard Smith via llvm-commits
> Sent: Wednesday, October 10, 2018 14:10
> To: llvm-commits at lists.llvm.org
> Subject: [llvm] r344184 - Support for remapping profile data when
> symbols change, for
>
> Author: rsmith
> Date: Wed Oct 10 14:09:37 2018
> New Revision: 344184
>
> URL: http://llvm.org/viewvc/llvm-project?rev=344184&view=rev
> Log:
> Support for remapping profile data when symbols change, for
> instrumentation-based profiling.
>
> Reviewers: davidxl, tejohnson, dlj, erik.pilkington
>
> Subscribers: llvm-commits
>
> Differential Revision: https://reviews.llvm.org/D51247
>
> Modified:
> llvm/trunk/include/llvm/ProfileData/InstrProfReader.h
> llvm/trunk/lib/ProfileData/InstrProfReader.cpp
> llvm/trunk/unittests/ProfileData/InstrProfTest.cpp
>
> Modified: llvm/trunk/include/llvm/ProfileData/InstrProfReader.h
> URL: http://llvm.org/viewvc/llvm-
> project/llvm/trunk/include/llvm/ProfileData/InstrProfReader.h?rev=34418
> 4&r1=344183&r2=344184&view=diff
> =======================================================================
> =======
> --- llvm/trunk/include/llvm/ProfileData/InstrProfReader.h (original)
> +++ llvm/trunk/include/llvm/ProfileData/InstrProfReader.h Wed Oct 10
> 14:09:37 2018
> @@ -349,12 +349,17 @@ using OnDiskHashTableImplV3 =
> OnDiskIterableChainedHashTable<InstrProfLookupTrait>;
>
> template <typename HashTableImpl>
> +class InstrProfReaderItaniumRemapper;
> +
> +template <typename HashTableImpl>
> class InstrProfReaderIndex : public InstrProfReaderIndexBase {
> private:
> std::unique_ptr<HashTableImpl> HashTable;
> typename HashTableImpl::data_iterator RecordIterator;
> uint64_t FormatVersion;
>
> + friend class InstrProfReaderItaniumRemapper<HashTableImpl>;
> +
> public:
> InstrProfReaderIndex(const unsigned char *Buckets,
> const unsigned char *const Payload,
> @@ -386,13 +391,26 @@ public:
> }
> };
>
> +/// Name matcher supporting fuzzy matching of symbol names to names in
> profiles.
> +class InstrProfReaderRemapper {
> +public:
> + virtual ~InstrProfReaderRemapper() {}
> + virtual Error populateRemappings() { return Error::success(); }
> + virtual Error getRecords(StringRef FuncName,
> + ArrayRef<NamedInstrProfRecord> &Data) = 0;
> +};
> +
> /// Reader for the indexed binary instrprof format.
> class IndexedInstrProfReader : public InstrProfReader {
> private:
> /// The profile data file contents.
> std::unique_ptr<MemoryBuffer> DataBuffer;
> + /// The profile remapping file contents.
> + std::unique_ptr<MemoryBuffer> RemappingBuffer;
> /// The index into the profile data.
> std::unique_ptr<InstrProfReaderIndexBase> Index;
> + /// The profile remapping file contents.
> + std::unique_ptr<InstrProfReaderRemapper> Remapper;
> /// Profile summary data.
> std::unique_ptr<ProfileSummary> Summary;
> // Index to the current record in the record array.
> @@ -404,8 +422,11 @@ private:
> const unsigned char *Cur);
>
> public:
> - IndexedInstrProfReader(std::unique_ptr<MemoryBuffer> DataBuffer)
> - : DataBuffer(std::move(DataBuffer)), RecordIndex(0) {}
> + IndexedInstrProfReader(
> + std::unique_ptr<MemoryBuffer> DataBuffer,
> + std::unique_ptr<MemoryBuffer> RemappingBuffer = nullptr)
> + : DataBuffer(std::move(DataBuffer)),
> + RemappingBuffer(std::move(RemappingBuffer)), RecordIndex(0) {}
> IndexedInstrProfReader(const IndexedInstrProfReader &) = delete;
> IndexedInstrProfReader &operator=(const IndexedInstrProfReader &) =
> delete;
>
> @@ -434,10 +455,11 @@ public:
>
> /// Factory method to create an indexed reader.
> static Expected<std::unique_ptr<IndexedInstrProfReader>>
> - create(const Twine &Path);
> + create(const Twine &Path, const Twine &RemappingPath = "");
>
> static Expected<std::unique_ptr<IndexedInstrProfReader>>
> - create(std::unique_ptr<MemoryBuffer> Buffer);
> + create(std::unique_ptr<MemoryBuffer> Buffer,
> + std::unique_ptr<MemoryBuffer> RemappingBuffer = nullptr);
>
> // Used for testing purpose only.
> void setValueProfDataEndianness(support::endianness Endianness) {
>
> Modified: llvm/trunk/lib/ProfileData/InstrProfReader.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/llvm/trunk/lib/ProfileData/InstrProfReader.cpp?rev=344184&r1=34
> 4183&r2=344184&view=diff
> =======================================================================
> =======
> --- llvm/trunk/lib/ProfileData/InstrProfReader.cpp (original)
> +++ llvm/trunk/lib/ProfileData/InstrProfReader.cpp Wed Oct 10 14:09:37
> 2018
> @@ -14,6 +14,7 @@
>
> #include "llvm/ProfileData/InstrProfReader.h"
> #include "llvm/ADT/ArrayRef.h"
> +#include "llvm/ADT/DenseMap.h"
> #include "llvm/ADT/STLExtras.h"
> #include "llvm/ADT/StringRef.h"
> #include "llvm/IR/ProfileSummary.h"
> @@ -23,6 +24,7 @@
> #include "llvm/Support/Error.h"
> #include "llvm/Support/ErrorOr.h"
> #include "llvm/Support/MemoryBuffer.h"
> +#include "llvm/Support/SymbolRemappingReader.h"
> #include "llvm/Support/SwapByteOrder.h"
> #include <algorithm>
> #include <cctype>
> @@ -88,16 +90,29 @@ InstrProfReader::create(std::unique_ptr<
> }
>
> Expected<std::unique_ptr<IndexedInstrProfReader>>
> -IndexedInstrProfReader::create(const Twine &Path) {
> +IndexedInstrProfReader::create(const Twine &Path, const Twine
> &RemappingPath) {
> // Set up the buffer to read.
> auto BufferOrError = setupMemoryBuffer(Path);
> if (Error E = BufferOrError.takeError())
> return std::move(E);
> - return
> IndexedInstrProfReader::create(std::move(BufferOrError.get()));
> +
> + // Set up the remapping buffer if requested.
> + std::unique_ptr<MemoryBuffer> RemappingBuffer;
> + std::string RemappingPathStr = RemappingPath.str();
> + if (!RemappingPathStr.empty()) {
> + auto RemappingBufferOrError = setupMemoryBuffer(RemappingPathStr);
> + if (Error E = RemappingBufferOrError.takeError())
> + return std::move(E);
> + RemappingBuffer = std::move(RemappingBufferOrError.get());
> + }
> +
> + return
> IndexedInstrProfReader::create(std::move(BufferOrError.get()),
> + std::move(RemappingBuffer));
> }
>
> Expected<std::unique_ptr<IndexedInstrProfReader>>
> -IndexedInstrProfReader::create(std::unique_ptr<MemoryBuffer> Buffer) {
> +IndexedInstrProfReader::create(std::unique_ptr<MemoryBuffer> Buffer,
> + std::unique_ptr<MemoryBuffer>
> RemappingBuffer) {
> // Sanity check the buffer.
> if (uint64_t(Buffer->getBufferSize()) >
> std::numeric_limits<unsigned>::max())
> return make_error<InstrProfError>(instrprof_error::too_large);
> @@ -105,7 +120,8 @@ IndexedInstrProfReader::create(std::uniq
> // Create the reader.
> if (!IndexedInstrProfReader::hasFormat(*Buffer))
> return make_error<InstrProfError>(instrprof_error::bad_magic);
> - auto Result =
> llvm::make_unique<IndexedInstrProfReader>(std::move(Buffer));
> + auto Result = llvm::make_unique<IndexedInstrProfReader>(
> + std::move(Buffer), std::move(RemappingBuffer));
>
> // Initialize the reader and return the result.
> if (Error E = initializeReader(*Result))
> @@ -587,6 +603,124 @@ InstrProfReaderIndex<HashTableImpl>::Ins
> RecordIterator = HashTable->data_begin();
> }
>
> +namespace {
> +/// A remapper that does not apply any remappings.
> +class InstrProfReaderNullRemapper : public InstrProfReaderRemapper {
> + InstrProfReaderIndexBase &Underlying;
> +
> +public:
> + InstrProfReaderNullRemapper(InstrProfReaderIndexBase &Underlying)
> + : Underlying(Underlying) {}
> +
> + Error getRecords(StringRef FuncName,
> + ArrayRef<NamedInstrProfRecord> &Data) override {
> + return Underlying.getRecords(FuncName, Data);
> + }
> +};
> +}
> +
> +/// A remapper that applies remappings based on a symbol remapping
> file.
> +template <typename HashTableImpl>
> +class llvm::InstrProfReaderItaniumRemapper
> + : public InstrProfReaderRemapper {
> +public:
> + InstrProfReaderItaniumRemapper(
> + std::unique_ptr<MemoryBuffer> RemapBuffer,
> + InstrProfReaderIndex<HashTableImpl> &Underlying)
> + : RemapBuffer(std::move(RemapBuffer)), Underlying(Underlying) {
> + }
> +
> + /// Extract the original function name from a PGO function name.
> + static StringRef extractName(StringRef Name) {
> + // We can have multiple :-separated pieces; there can be pieces
> both
> + // before and after the mangled name. Find the first part that
> starts
> + // with '_Z'; we'll assume that's the mangled name we want.
> + std::pair<StringRef, StringRef> Parts = {StringRef(), Name};
> + while (true) {
> + Parts = Parts.second.split(':');
> + if (Parts.first.startswith("_Z"))
> + return Parts.first;
> + if (Parts.second.empty())
> + return Name;
> + }
> + }
> +
> + /// Given a mangled name extracted from a PGO function name, and a
> new
> + /// form for that mangled name, reconstitute the name.
> + static void reconstituteName(StringRef OrigName, StringRef
> ExtractedName,
> + StringRef Replacement,
> + SmallVectorImpl<char> &Out) {
> + Out.reserve(OrigName.size() + Replacement.size() -
> ExtractedName.size());
> + Out.insert(Out.end(), OrigName.begin(), ExtractedName.begin());
> + Out.insert(Out.end(), Replacement.begin(), Replacement.end());
> + Out.insert(Out.end(), ExtractedName.end(), OrigName.end());
> + }
> +
> + Error populateRemappings() override {
> + if (Error E = Remappings.read(*RemapBuffer))
> + return E;
> + for (StringRef Name : Underlying.HashTable->keys()) {
> + StringRef RealName = extractName(Name);
> + if (auto Key = Remappings.insert(RealName)) {
> + // FIXME: We could theoretically map the same equivalence
> class to
> + // multiple names in the profile data. If that happens, we
> should
> + // return NamedInstrProfRecords from all of them.
> + MappedNames.insert({Key, RealName});
> + }
> + }
> + return Error::success();
> + }
> +
> + Error getRecords(StringRef FuncName,
> + ArrayRef<NamedInstrProfRecord> &Data) override {
> + StringRef RealName = extractName(FuncName);
> + if (auto Key = Remappings.lookup(RealName)) {
> + StringRef Remapped = MappedNames.lookup(Key);
> + if (!Remapped.empty()) {
> + if (RealName.begin() == FuncName.begin() &&
> + RealName.end() == FuncName.end())
> + FuncName = Remapped;
> + else {
> + // Try rebuilding the name from the given remapping.
> + SmallString<256> Reconstituted;
> + reconstituteName(FuncName, RealName, Remapped,
> Reconstituted);
> + Error E = Underlying.getRecords(Reconstituted, Data);
> + if (!E)
> + return E;
> +
> + // If we failed because the name doesn't exist, fall back to
> asking
> + // about the original name.
> + if (Error Unhandled = handleErrors(
> + std::move(E), [](std::unique_ptr<InstrProfError>
> Err) {
> + return Err->get() ==
> instrprof_error::unknown_function
> + ? Error::success()
> + : Error(std::move(Err));
> + }))
> + return Unhandled;
> + }
> + }
> + }
> + return Underlying.getRecords(FuncName, Data);
> + }
> +
> +private:
> + /// The memory buffer containing the remapping configuration.
> Remappings
> + /// holds pointers into this buffer.
> + std::unique_ptr<MemoryBuffer> RemapBuffer;
> +
> + /// The mangling remapper.
> + SymbolRemappingReader Remappings;
> +
> + /// Mapping from mangled name keys to the name used for the key in
> the
> + /// profile data.
> + /// FIXME: Can we store a location within the on-disk hash table
> instead of
> + /// redoing lookup?
> + DenseMap<SymbolRemappingReader::Key, StringRef> MappedNames;
> +
> + /// The real profile data reader.
> + InstrProfReaderIndex<HashTableImpl> &Underlying;
> +};
> +
> bool IndexedInstrProfReader::hasFormat(const MemoryBuffer &DataBuffer)
> {
> using namespace support;
>
> @@ -683,10 +817,22 @@ Error IndexedInstrProfReader::readHeader
> uint64_t HashOffset = endian::byte_swap<uint64_t, little>(Header-
> >HashOffset);
>
> // The rest of the file is an on disk hash table.
> - InstrProfReaderIndexBase *IndexPtr = nullptr;
> - IndexPtr = new InstrProfReaderIndex<OnDiskHashTableImplV3>(
> - Start + HashOffset, Cur, Start, HashType, FormatVersion);
> - Index.reset(IndexPtr);
> + auto IndexPtr =
> + llvm::make_unique<InstrProfReaderIndex<OnDiskHashTableImplV3>>(
> + Start + HashOffset, Cur, Start, HashType, FormatVersion);
> +
> + // Load the remapping table now if requested.
> + if (RemappingBuffer) {
> + Remapper = llvm::make_unique<
> + InstrProfReaderItaniumRemapper<OnDiskHashTableImplV3>>(
> + std::move(RemappingBuffer), *IndexPtr);
> + if (Error E = Remapper->populateRemappings())
> + return E;
> + } else {
> + Remapper =
> llvm::make_unique<InstrProfReaderNullRemapper>(*IndexPtr);
> + }
> + Index = std::move(IndexPtr);
> +
> return success();
> }
>
> @@ -707,7 +853,7 @@ Expected<InstrProfRecord>
> IndexedInstrProfReader::getInstrProfRecord(StringRef FuncName,
> uint64_t FuncHash) {
> ArrayRef<NamedInstrProfRecord> Data;
> - Error Err = Index->getRecords(FuncName, Data);
> + Error Err = Remapper->getRecords(FuncName, Data);
> if (Err)
> return std::move(Err);
> // Found it. Look for counters with the right hash.
>
> Modified: llvm/trunk/unittests/ProfileData/InstrProfTest.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/llvm/trunk/unittests/ProfileData/InstrProfTest.cpp?rev=344184&r
> 1=344183&r2=344184&view=diff
> =======================================================================
> =======
> --- llvm/trunk/unittests/ProfileData/InstrProfTest.cpp (original)
> +++ llvm/trunk/unittests/ProfileData/InstrProfTest.cpp Wed Oct 10
> 14:09:37 2018
> @@ -42,8 +42,10 @@ struct InstrProfTest : ::testing::Test {
>
> void SetUp() { Writer.setOutputSparse(false); }
>
> - void readProfile(std::unique_ptr<MemoryBuffer> Profile) {
> - auto ReaderOrErr =
> IndexedInstrProfReader::create(std::move(Profile));
> + void readProfile(std::unique_ptr<MemoryBuffer> Profile,
> + std::unique_ptr<MemoryBuffer> Remapping = nullptr)
> {
> + auto ReaderOrErr =
> IndexedInstrProfReader::create(std::move(Profile),
> +
> std::move(Remapping));
> EXPECT_THAT_ERROR(ReaderOrErr.takeError(), Succeeded());
> Reader = std::move(ReaderOrErr.get());
> }
> @@ -990,6 +992,44 @@ TEST_P(MaybeSparseInstrProfTest, instr_p
> }
> }
>
> +TEST_P(MaybeSparseInstrProfTest, remapping_test) {
> + Writer.addRecord({"_Z3fooi", 0x1234, {1, 2, 3, 4}}, Err);
> + Writer.addRecord({"file:_Z3barf", 0x567, {5, 6, 7}}, Err);
> + auto Profile = Writer.writeBuffer();
> + readProfile(std::move(Profile), llvm::MemoryBuffer::getMemBuffer(R"(
> + type i l
> + name 3bar 4quux
> + )"));
> +
> + std::vector<uint64_t> Counts;
> + for (StringRef FooName : {"_Z3fooi", "_Z3fool"}) {
> + EXPECT_THAT_ERROR(Reader->getFunctionCounts(FooName, 0x1234,
> Counts),
> + Succeeded());
> + ASSERT_EQ(4u, Counts.size());
> + EXPECT_EQ(1u, Counts[0]);
> + EXPECT_EQ(2u, Counts[1]);
> + EXPECT_EQ(3u, Counts[2]);
> + EXPECT_EQ(4u, Counts[3]);
> + }
> +
> + for (StringRef BarName : {"file:_Z3barf", "file:_Z4quuxf"}) {
> + EXPECT_THAT_ERROR(Reader->getFunctionCounts(BarName, 0x567,
> Counts),
> + Succeeded());
> + ASSERT_EQ(3u, Counts.size());
> + EXPECT_EQ(5u, Counts[0]);
> + EXPECT_EQ(6u, Counts[1]);
> + EXPECT_EQ(7u, Counts[2]);
> + }
> +
> + for (StringRef BadName : {"_Z3foof", "_Z4quuxi", "_Z3barl", "",
> "_ZZZ",
> + "_Z3barf", "otherfile:_Z4quuxf"}) {
> + EXPECT_THAT_ERROR(Reader->getFunctionCounts(BadName, 0x1234,
> Counts),
> + Failed());
> + EXPECT_THAT_ERROR(Reader->getFunctionCounts(BadName, 0x567,
> Counts),
> + Failed());
> + }
> +}
> +
> TEST_F(SparseInstrProfTest, preserve_no_records) {
> Writer.addRecord({"foo", 0x1234, {0}}, Err);
> Writer.addRecord({"bar", 0x4321, {0, 0}}, Err);
>
>
> _______________________________________________
> 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