Re: [PATCH] D84782: [PGO] Include the mem ops into the function hash.

On Wed, Aug 5, 2020 at 6:26 PM Hiroshi Yamauchi via llvm-commits wrote: > > > > On Wed, Aug 5, 2020 at 8:30 AM Hans Wennborg via Phabricator > wrote: >> >> hans added a comment. >> >> In D84782#2191429 , @yamauchi wrote: >> >> > In D84782#2191243

Re: [PATCH] D84782: [PGO] Include the mem ops into the function hash.

On Wed, Aug 5, 2020 at 8:30 AM Hans Wennborg via Phabricator < revi...@reviews.llvm.org> wrote: > hans added a comment. > > In D84782#2191429 , @yamauchi > wrote: > > > In D84782#2191243 , @MaskRay > wrote: > > > >>

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

hans added a comment. In D84782#2191429 , @yamauchi wrote: > In D84782#2191243 , @MaskRay wrote: > >> @yamauchi Do we need cd890944ad344b1b8cac58332ab11c9eec6b61e9 >>

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added a comment. In D84782#2191243 , @MaskRay wrote: > @yamauchi Do we need cd890944ad344b1b8cac58332ab11c9eec6b61e9 > and > 3d6f53018f845e893ad34f64ff2851a2e5c3ba1d >

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

MaskRay added a subscriber: hans. MaskRay added a comment. @yamauchi Do we need cd890944ad344b1b8cac58332ab11c9eec6b61e9 and 3d6f53018f845e893ad34f64ff2851a2e5c3ba1d

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added a comment. Yup, -pgo-instr-old-cfg-hashing=true to revert back to the old hashing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 ___ cfe-commits

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

MaskRay added a comment. It is also a convention to explain what has changed compared with the initial land. As a hindsight, the option `-pgo-instr-old-cfg-hashing` should probably have been mentioned in the description. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

MaskRay closed this revision. MaskRay added a comment. Relanded as 3d6f53018f845e893ad34f64ff2851a2e5c3ba1d @yamauchi For a reland, you still need to include `Differential Revision: ` so that the differential can be closed

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl added a comment. lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added a comment. Latest update diff: < + std::vector Data; < + for (unsigned I = 0; I < 8; ++I) < +Data.push_back((uint8_t)(Num >> (I * 8))); --- > + uint8_t Data[8]; > + support::endian::write64le(Data, Num); Comment at:

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi updated this revision to Diff 281764. yamauchi added a comment. Use endian::write64le. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 Files: clang/test/CodeGen/Inputs/thinlto_expect1.proftext

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added a comment. Here's the diff in the latest update: < +union { < + uint64_t N; < + uint8_t C[8]; < +} Data; < +Data.N = (uint64_t)SIVisitor.getNumOfSelectInsts(); < +JCH.update(Data.C); < +Data.N =

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:661 +} Data; +Data.N = (uint64_t)SIVisitor.getNumOfSelectInsts(); +JCH.update(Data.C); MaskRay wrote: > MaskRay wrote: > > `#include

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi updated this revision to Diff 281760. yamauchi added a comment. Fix the endianness issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 Files:

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

MaskRay added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:661 +} Data; +Data.N = (uint64_t)SIVisitor.getNumOfSelectInsts(); +JCH.update(Data.C); MaskRay wrote: > `#include "llvm/Support/Endian.h"` > > and

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

MaskRay added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:661 +} Data; +Data.N = (uint64_t)SIVisitor.getNumOfSelectInsts(); +JCH.update(Data.C); `#include "llvm/Support/Endian.h"` and use

Re: [PATCH] D84782: [PGO] Include the mem ops into the function hash.

right. It occurred to me during review, but did not think of the hard coded values in proftext depends on LE. On Wed, Jul 29, 2020 at 3:04 PM Hiroshi Yamauchi via Phabricator < revi...@reviews.llvm.org> wrote: > yamauchi added a comment. > > Builtbot failure: >

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added a comment. Builtbot failure: http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/51984 I think it's an endian issue in the hash computation. Will revert. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG120e66b3418b: [PGO] Include the mem ops into the function hash. (authored by yamauchi). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl accepted this revision. davidxl added a comment. lgtm Just realized that we need a test case to show it fixes the original issue (existence with memop --> different hash). Ok as a follow up . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi updated this revision to Diff 281716. yamauchi added a comment. Address comments. PTAL. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 Files: clang/test/CodeGen/Inputs/thinlto_expect1.proftext

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added inline comments. Comment at: llvm/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext:20 18 12 davidxl wrote: > nit: change 12 to a different value say 6.(otherwise if the option is an nop, > the test will still succeed). Done.

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

MaskRay accepted this revision. MaskRay added a comment. LGTM as well with @davidxl's comments addressed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 ___

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. lgtm (with the small test enhancement) Comment at: llvm/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext:20 18 12 nit: change 12 to a

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added inline comments. Comment at: llvm/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext:1 # IR level Instrumentation Flag :ir davidxl wrote: > We can convert this test case to cover the new option introduced: > > basically create

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi updated this revision to Diff 281708. yamauchi added a comment. Address comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 Files: clang/test/CodeGen/Inputs/thinlto_expect1.proftext

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl added inline comments. Comment at: llvm/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext:1 # IR level Instrumentation Flag :ir We can convert this test case to cover the new option introduced: basically create another profile entry

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi updated this revision to Diff 281673. yamauchi added a comment. Rebase past D84865 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 Files:

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added a comment. In D84782#2180626 , @davidxl wrote: > changes like in llvm/test/Transforms/PGOProfile/PR41279.ll etc can be > independently committed. Uploaded D84865 for this. Repository: rG LLVM Github

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:266 +PGOOldCFGHashing("pgo-instr-old-cfg-hashing", cl::init(false), cl::Hidden, + cl::desc("Use the old CFG function hashing.")); +

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi updated this revision to Diff 281626. yamauchi added a comment. Address comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 Files: clang/test/CodeGen/Inputs/thinlto_expect1.proftext

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

MaskRay added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:266 +PGOOldCFGHashing("pgo-instr-old-cfg-hashing", cl::init(false), cl::Hidden, + cl::desc("Use the old CFG function hashing.")); + Nit:

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl added a comment. changes like in llvm/test/Transforms/PGOProfile/PR41279.ll etc can be independently committed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added a comment. In D84782#2180578 , @davidxl wrote: > Can you split out the test changes and commit it separately? How exactly? Do you mean commit the non-test part without flipping the flag and then flip it with the test changes?

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi updated this revision to Diff 281426. yamauchi added a comment. Rebase (converting one more test) and fixing a typo and whitespace. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 Files:

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl added a comment. Can you split out the test changes and commit it separately? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 ___ cfe-commits mailing

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added a comment. In D84782#2180432 , @davidxl wrote: > On version bump --- looks like FrontPGO has been bumping version for hashing > changes while IR PGO had not, so it is ok to leave the version unchanged > (current situation is also

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi updated this revision to Diff 281417. yamauchi added a comment. Add a flag to back out to the old hashing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 Files:

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added a comment. > Many tests can also be enhanced to filter out the hash details if they are > not part the main test. Done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi updated this revision to Diff 281407. yamauchi added a comment. Shift the higher 32 bits by 28 bits and add. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/ https://reviews.llvm.org/D84782 Files:

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl added a comment. On version bump --- looks like FrontPGO has been bumping version for hashing changes while IR PGO had not, so it is ok to leave the version unchanged (current situation is also confusing as the version of IR and FE PGO are mixed). On the other hand, just in case, we

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

davidxl added a comment. We may also need to bump both the raw and index format version with this change. For the profile use side, we also need to keep the hashing scheme of the older version (in profile-use side). More details to come. Many tests can also be enhanced to filter out the hash

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:660 // information. - FunctionHash = (uint64_t)SIVisitor.getNumOfSelectInsts() << 56 | - (uint64_t)ValueSites[IPVK_IndirectCallTarget].size() << 48 | -

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:623 // Compute Hash value for the CFG: the lower 32 bits are CRC32 of the index -// value of each BB in the CFG. The higher 32 bits record the number of edges. +// value of

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

MaskRay added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:623 // Compute Hash value for the CFG: the lower 32 bits are CRC32 of the index -// value of each BB in the CFG. The higher 32 bits record the number of edges. +// value of

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:642 // Hash format for context sensitive profile. Reserve 4 bits for other // information. davidxl wrote: > The comment looks stale. I think the 4 bits

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

yamauchi updated this revision to Diff 281364. yamauchi added a comment. Herald added subscribers: cfe-commits, dexonsmith, steven_wu. Herald added a project: clang. Use JamCRC. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84782/new/