Author: serge-sans-paille Date: 2020-05-27T09:15:21+02:00 New Revision: de02a75e398415bad4df27b4547c25b896c8bf3b
URL: https://github.com/llvm/llvm-project/commit/de02a75e398415bad4df27b4547c25b896c8bf3b DIFF: https://github.com/llvm/llvm-project/commit/de02a75e398415bad4df27b4547c25b896c8bf3b.diff LOG: [PGO] Fix computation of function Hash And bump its version number accordingly. This is a patched recommit of 7c298c104bfe725d4315926a656263e8a5ac3054 Previous hash implementation was incorrectly passing an uint64_t, that got converted to an uint8_t, to finalize the hash computation. This led to different functions having the same hash if they only differ by the remaining statements, which is incorrect. Added a new test case that trivially tests that a small function change is reflected in the hash value. Not that as this patch fixes the hash computation, it would invalidate all hashes computed before that patch applies, this is why we bumped the version number. Update profile data hash entries due to hash function update, except for binary version, in which case we keep the buggy behavior for backward compatibility. Differential Revision: https://reviews.llvm.org/D79961 Added: clang/test/Profile/Inputs/c-general.profdata.v5 clang/test/Profile/c-collision.c Modified: clang/docs/ReleaseNotes.rst clang/lib/CodeGen/CodeGenPGO.cpp clang/test/Profile/Inputs/c-counter-overflows.proftext clang/test/Profile/Inputs/c-general.proftext clang/test/Profile/Inputs/c-unprofiled-blocks.proftext clang/test/Profile/Inputs/cxx-rangefor.proftext clang/test/Profile/Inputs/cxx-throws.proftext clang/test/Profile/Inputs/misexpect-switch-default.proftext clang/test/Profile/Inputs/misexpect-switch-nonconst.proftext clang/test/Profile/Inputs/misexpect-switch.proftext clang/test/Profile/c-general.c llvm/include/llvm/ProfileData/InstrProf.h llvm/include/llvm/ProfileData/InstrProfData.inc Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index c38ff0e36790..571b54904754 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -82,6 +82,10 @@ Non-comprehensive list of changes in this release linker. If the user links the program with the ``clang`` or ``clang-cl`` drivers, the driver will pass this flag for them. +- Clang's profile files generated through ``-fprofile-instr-generate`` are using + a fixed hashing algorithm that prevents some collision when loading + out-of-date profile informations. Clang can still read old profile files. + New Compiler Flags ------------------ diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp index 3c91a04d5464..e810f608ab78 100644 --- a/clang/lib/CodeGen/CodeGenPGO.cpp +++ b/clang/lib/CodeGen/CodeGenPGO.cpp @@ -52,9 +52,10 @@ void CodeGenPGO::setFuncName(llvm::Function *Fn) { enum PGOHashVersion : unsigned { PGO_HASH_V1, PGO_HASH_V2, + PGO_HASH_V3, // Keep this set to the latest hash version. - PGO_HASH_LATEST = PGO_HASH_V2 + PGO_HASH_LATEST = PGO_HASH_V3 }; namespace { @@ -122,7 +123,7 @@ class PGOHash { BinaryOperatorGE, BinaryOperatorEQ, BinaryOperatorNE, - // The preceding values are available with PGO_HASH_V2. + // The preceding values are available since PGO_HASH_V2. // Keep this last. It's for the static assert that follows. LastHashType @@ -144,7 +145,9 @@ static PGOHashVersion getPGOHashVersion(llvm::IndexedInstrProfReader *PGOReader, CodeGenModule &CGM) { if (PGOReader->getVersion() <= 4) return PGO_HASH_V1; - return PGO_HASH_V2; + if (PGOReader->getVersion() <= 5) + return PGO_HASH_V2; + return PGO_HASH_V3; } /// A RecursiveASTVisitor that fills a map of statements to PGO counters. @@ -288,7 +291,7 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> { return PGOHash::BinaryOperatorLAnd; if (BO->getOpcode() == BO_LOr) return PGOHash::BinaryOperatorLOr; - if (HashVersion == PGO_HASH_V2) { + if (HashVersion >= PGO_HASH_V2) { switch (BO->getOpcode()) { default: break; @@ -310,7 +313,7 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> { } } - if (HashVersion == PGO_HASH_V2) { + if (HashVersion >= PGO_HASH_V2) { switch (S->getStmtClass()) { default: break; @@ -747,13 +750,21 @@ uint64_t PGOHash::finalize() { return Working; // Check for remaining work in Working. - if (Working) - MD5.update(Working); + if (Working) { + // Keep the buggy behavior from v1 and v2 for backward-compatibility. This + // is buggy because it converts a uint64_t into an array of uint8_t. + if (HashVersion < PGO_HASH_V3) { + MD5.update({(uint8_t)Working}); + } else { + using namespace llvm::support; + uint64_t Swapped = endian::byte_swap<uint64_t, little>(Working); + MD5.update(llvm::makeArrayRef((uint8_t *)&Swapped, sizeof(Swapped))); + } + } // Finalize the MD5 and return the hash. llvm::MD5::MD5Result Result; MD5.final(Result); - using namespace llvm::support; return Result.low(); } diff --git a/clang/test/Profile/Inputs/c-counter-overflows.proftext b/clang/test/Profile/Inputs/c-counter-overflows.proftext index b2e5dd1d77ae..4d0287c78705 100644 --- a/clang/test/Profile/Inputs/c-counter-overflows.proftext +++ b/clang/test/Profile/Inputs/c-counter-overflows.proftext @@ -1,5 +1,5 @@ main -10111551811706059223 +7779561829442898616 8 1 68719476720 diff --git a/clang/test/Profile/Inputs/c-general.profdata.v5 b/clang/test/Profile/Inputs/c-general.profdata.v5 new file mode 100644 index 000000000000..435ef2b6ef1d Binary files /dev/null and b/clang/test/Profile/Inputs/c-general.profdata.v5 diff er diff --git a/clang/test/Profile/Inputs/c-general.proftext b/clang/test/Profile/Inputs/c-general.proftext index c0d03b4b755e..18ad2050fe1a 100644 --- a/clang/test/Profile/Inputs/c-general.proftext +++ b/clang/test/Profile/Inputs/c-general.proftext @@ -7,7 +7,7 @@ simple_loops 75 conditionals -4190663230902537370 +4904767535850050386 11 1 100 @@ -22,7 +22,7 @@ conditionals 100 early_exits -8265526549255474475 +2880354649761471549 9 1 0 @@ -35,7 +35,7 @@ early_exits 0 jumps -15872630527555456493 +15051420506203462683 22 1 1 @@ -61,7 +61,7 @@ jumps 9 switches -11892326508727782373 +43242458792028222 19 1 1 @@ -84,7 +84,7 @@ switches 0 big_switch -16933280399284440835 +13144136522122330070 17 1 32 @@ -117,7 +117,7 @@ boolean_operators 50 boolop_loops -11270260636676715317 +12402604614320574815 9 1 50 @@ -137,7 +137,7 @@ conditional_operator 1 do_fallthrough -6898770640283947069 +8714614136504380050 4 1 10 diff --git a/clang/test/Profile/Inputs/c-unprofiled-blocks.proftext b/clang/test/Profile/Inputs/c-unprofiled-blocks.proftext index ef7f653811fb..d880663fed32 100644 --- a/clang/test/Profile/Inputs/c-unprofiled-blocks.proftext +++ b/clang/test/Profile/Inputs/c-unprofiled-blocks.proftext @@ -1,5 +1,5 @@ never_called -5644096560937528444 +6820425066224770721 9 0 0 @@ -17,7 +17,7 @@ main 1 dead_code -9636018207904213947 +5254464978620792806 10 1 0 diff --git a/clang/test/Profile/Inputs/cxx-rangefor.proftext b/clang/test/Profile/Inputs/cxx-rangefor.proftext index b59729207859..d41205bbde14 100644 --- a/clang/test/Profile/Inputs/cxx-rangefor.proftext +++ b/clang/test/Profile/Inputs/cxx-rangefor.proftext @@ -1,5 +1,5 @@ _Z9range_forv -6169071350249721981 +8789831523895825398 5 1 4 diff --git a/clang/test/Profile/Inputs/cxx-throws.proftext b/clang/test/Profile/Inputs/cxx-throws.proftext index 32fcf5d50cd4..043dea08c728 100644 --- a/clang/test/Profile/Inputs/cxx-throws.proftext +++ b/clang/test/Profile/Inputs/cxx-throws.proftext @@ -1,5 +1,5 @@ _Z6throwsv -340120998528097520 +18172607911962830854 9 1 100 diff --git a/clang/test/Profile/Inputs/misexpect-switch-default.proftext b/clang/test/Profile/Inputs/misexpect-switch-default.proftext index 7b2d59781a1d..533da9176523 100644 --- a/clang/test/Profile/Inputs/misexpect-switch-default.proftext +++ b/clang/test/Profile/Inputs/misexpect-switch-default.proftext @@ -1,6 +1,6 @@ main # Func Hash: -8712453512413296413 +8734802134600123338 # Num Counters: 9 # Counter Values: diff --git a/clang/test/Profile/Inputs/misexpect-switch-nonconst.proftext b/clang/test/Profile/Inputs/misexpect-switch-nonconst.proftext index 52b7b70cab9a..8e8db667d329 100644 --- a/clang/test/Profile/Inputs/misexpect-switch-nonconst.proftext +++ b/clang/test/Profile/Inputs/misexpect-switch-nonconst.proftext @@ -1,6 +1,6 @@ main # Func Hash: -1965403898329309329 +3721743393642630379 # Num Counters: 10 # Counter Values: diff --git a/clang/test/Profile/Inputs/misexpect-switch.proftext b/clang/test/Profile/Inputs/misexpect-switch.proftext index ce4c96b3e3a6..ce41cd0552d3 100644 --- a/clang/test/Profile/Inputs/misexpect-switch.proftext +++ b/clang/test/Profile/Inputs/misexpect-switch.proftext @@ -1,6 +1,6 @@ main # Func Hash: -1965403898329309329 +872687477373597607 # Num Counters: 9 # Counter Values: diff --git a/clang/test/Profile/c-collision.c b/clang/test/Profile/c-collision.c new file mode 100644 index 000000000000..fabecd752b4e --- /dev/null +++ b/clang/test/Profile/c-collision.c @@ -0,0 +1,22 @@ +// Test that a slight change in the code leads to a diff erent hash. +// RUN: %clang_cc1 -UEXTRA -triple x86_64-unknown-linux-gnu -main-file-name c-collision.c %s -o - -emit-llvm -fprofile-instrument=clang | FileCheck %s --check-prefix=CHECK-NOEXTRA +// RUN: %clang_cc1 -DEXTRA -triple x86_64-unknown-linux-gnu -main-file-name c-collision.c %s -o - -emit-llvm -fprofile-instrument=clang | FileCheck %s --check-prefix=CHECK-EXTRA + +// CHECK-NOEXTRA: @__profd_foo = private global { {{.*}} } { i64 6699318081062747564, i64 7156072912471487002, +// CHECK-EXTRA: @__profd_foo = private global { {{.*}} } { i64 6699318081062747564, i64 -4383447408116050035, + +extern int bar; +void foo() { + if (bar) { + } + if (bar) { + } + if (bar) { + if (bar) { +#ifdef EXTRA + if (bar) { + } +#endif + } + } +} diff --git a/clang/test/Profile/c-general.c b/clang/test/Profile/c-general.c index 22b4288a5fd6..a7f03e872881 100644 --- a/clang/test/Profile/c-general.c +++ b/clang/test/Profile/c-general.c @@ -4,6 +4,7 @@ // RUN: llvm-profdata merge %S/Inputs/c-general.proftext -o %t.profdata // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-general.c %s -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata | FileCheck -allow-deprecated-dag-overlap -check-prefix=PGOUSE %s +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-general.c %s -o - -emit-llvm -fprofile-instrument-use-path=%S/Inputs/c-general.profdata.v5 | FileCheck -allow-deprecated-dag-overlap -check-prefix=PGOUSE %s // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-general.c %s -o - -emit-llvm -fprofile-instrument-use-path=%S/Inputs/c-general.profdata.v3 | FileCheck -allow-deprecated-dag-overlap -check-prefix=PGOUSE %s // Also check compatibility with older profiles. // RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-general.c %s -o - -emit-llvm -fprofile-instrument-use-path=%S/Inputs/c-general.profdata.v1 | FileCheck -allow-deprecated-dag-overlap -check-prefix=PGOUSE %s diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h index cdd50d2d5ebc..62a0c6955708 100644 --- a/llvm/include/llvm/ProfileData/InstrProf.h +++ b/llvm/include/llvm/ProfileData/InstrProf.h @@ -979,6 +979,9 @@ enum ProfVersion { Version4 = 4, // In this version, the frontend PGO stable hash algorithm defaults to V2. Version5 = 5, + // In this version, the frontend PGO stable hash algorithm got fixed and + // may produce hashes diff erent from Version5. + Version6 = 6, // The current version is 5. CurrentVersion = INSTR_PROF_INDEX_VERSION }; diff --git a/llvm/include/llvm/ProfileData/InstrProfData.inc b/llvm/include/llvm/ProfileData/InstrProfData.inc index 5e5f4ff941f3..a6913527e67f 100644 --- a/llvm/include/llvm/ProfileData/InstrProfData.inc +++ b/llvm/include/llvm/ProfileData/InstrProfData.inc @@ -657,7 +657,7 @@ serializeValueProfDataFrom(ValueProfRecordClosure *Closure, /* Raw profile format version (start from 1). */ #define INSTR_PROF_RAW_VERSION 5 /* Indexed profile format version (start from 1). */ -#define INSTR_PROF_INDEX_VERSION 5 +#define INSTR_PROF_INDEX_VERSION 6 /* Coverage mapping format version (start from 0). */ #define INSTR_PROF_COVMAP_VERSION 3 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
