Page MenuHomePhabricator

[PGO] Fix computation of fuction Hash
ClosedPublic

Authored by serge-sans-paille on Thu, May 14, 1:04 PM.

Details

Summary

Previous 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 invalidates all hashes
computed before that patch applies, which could be an issue for large build
system that pre-compute the profile data and let client download them as part of
the build process.

Diff Detail

Event Timeline

Update test case

dmajor added a subscriber: dmajor.Fri, May 15, 3:53 AM

This patch is correct. A clarification of the description:

Previous implementation was incorrectly passing an integer, that got converted to a pointer, to finalize the hash computation.

Working (an uint64_t) was truncated to an uint8_t, converted to a one-element ArrayRef<uint8_t>, then passed to MD5.update

MaskRay added inline comments.Sun, May 17, 11:53 AM
clang/lib/CodeGen/CodeGenPGO.cpp
767–768

Can the using be deleted?

serge-sans-paille edited the summary of this revision. (Show Details)

Updated summary + removed namespace.

serge-sans-paille marked an inline comment as done.Mon, May 18, 1:17 AM
hans added a subscriber: hans.Mon, May 18, 1:21 AM

Nice fix, and nice test :-)

May I ask how you found this?

May I ask how you found this?

This was a long (and painful) quest when tracking https://bugzilla.redhat.com/show_bug.cgi?id=1827282

hans accepted this revision.Mon, May 25, 7:16 AM

lgtm

This revision is now accepted and ready to land.Mon, May 25, 7:16 AM
This revision was automatically updated to reflect the committed changes.
ormris added a subscriber: ormris.Mon, May 25, 10:20 AM

There's a few test failures related to this change on the PS4 bot. Could you take a look?

http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/68111

serge-sans-paille added a subscriber: bkramer.

@bkramer rightfully reverted this, faster than I could patch it :-) Here is the patch though, with the updated test case. @hans the only meaningful change is that I'm keeping the buggy behavior for hash v1 for compatibility reason.

This revision is now accepted and ready to land.Mon, May 25, 11:50 AM
Update profile data hash entries due to hash function update, unless the version
used is V1, in which case we keep the buggy behavior for backward compatibility.
MaskRay accepted this revision.Mon, May 25, 12:38 PM

Thanks!

@hans/ @MaskRay I'm starting to wonder if I should bump the version number of the hash function, so that clang could still read profile data generated before that patch?

Bump the version number to be compatible with existing profdata, in a similar fashion to v1/v2 transition.

hans added a comment.Tue, May 26, 1:28 AM

Bump the version number to be compatible with existing profdata, in a similar fashion to v1/v2 transition.

Did you forget to include some files? I don't see the bump anywhere.

clang/lib/CodeGen/CodeGenPGO.cpp
757

Maybe explicitly show the conversion to array of uint8_t here, to make it more clear what's going on.

With v3 version + Make cast explicit

hans added a comment.Tue, May 26, 4:25 AM

I worry that the hash version bump isn't complete. Doesn't the hash version used need to be read/written with the profile data file somewhere?

clang/lib/CodeGen/CodeGenPGO.cpp
148–150

I guess this needs an update now?

294

Should this be >= PGO_HASH_V2 now? And similarly below?

756

Maybe just "HashVersion < PGO_HASH_V3" would be simpler?

serge-sans-paille marked an inline comment as done.

Update version bump parts

Herald added a project: Restricted Project. · View Herald TranscriptTue, May 26, 6:10 AM
serge-sans-paille marked 3 inline comments as done.Tue, May 26, 6:28 AM
hans accepted this revision.Tue, May 26, 7:04 AM

lgtm

This revision was automatically updated to reflect the committed changes.

It looks like clang/test/Profile/Inputs/c-general.profdata.v5 is being read as v6 rather than v5. Can you double check?

It looks like clang/test/Profile/Inputs/c-general.profdata.v5 is being read as v6 rather than v5. Can you double check?

Yep, I'll have a look.