This is an archive of the discontinued LLVM Phabricator instance.

[llvm-profdata] Change instr prof counter overflow to saturate rather than discard
ClosedPublic

Authored by slingn on Nov 20 2015, 3:54 PM.

Details

Summary

This changes overflow handling during instrumentation profile merge. Rathar than throwing away records that would result in counter overflow, merged counts are instead clamped to the maximum representable value. A warning about counter overflow is still surfaced to the user as before.

Diff Detail

Event Timeline

slingn updated this revision to Diff 40841.Nov 20 2015, 3:54 PM
slingn retitled this revision from to [llvm-profdata] Change instr prof counter overflow to saturate rather than discard.
slingn updated this object.
slingn added reviewers: dnovillo, davidxl, silvas.
slingn added a subscriber: llvm-commits.
silvas added inline comments.Nov 20 2015, 8:02 PM
include/llvm/ProfileData/InstrProf.h
421

Is this the right check? Seems like it would incorrectly fire when adding Counts[I] == "CounterMax - 1" and Other.Counts[I] == "1" since we are overwriting Counts[I]
The cleanest thing might be to add an optional argument bool *ResultOverflowed = nullptr to SaturatingAdd.

davidxl added inline comments.Nov 20 2015, 8:35 PM
include/llvm/ProfileData/InstrProf.h
421

Or perhaps just

NewCount = SaturatingAdd(....);
if (NewCount - Counts[I] != Other.Counts[I)

Result = ...

Counts[I] = NewCount;

Quoted Text

silvas added inline comments.Nov 20 2015, 8:47 PM
include/llvm/ProfileData/InstrProf.h
421

I like that!

slingn updated this revision to Diff 40934.Nov 23 2015, 9:01 AM

Updated for davidxl and silvas comments.

slingn marked 3 inline comments as done.Nov 23 2015, 9:01 AM
davidxl added inline comments.Nov 23 2015, 9:20 AM
lib/ProfileData/InstrProfWriter.cpp
118

This line does not look right. Is it needed?

slingn updated this revision to Diff 40946.Nov 23 2015, 9:51 AM
slingn marked an inline comment as done.

Updated for davidxl's comment.

include/llvm/ProfileData/InstrProf.h
421

The inline check for SaturatingAdd() overflow works well here, but thinking about the final weighted profile change - we need a way to check for SaturatingMultiply() overflow at multiple points in the code.

I'll follow up this change with Sean's suggestion of adding a third optional argument to SaturatingAdd()/SaturatingMultiply() so the overflow check code can be encapsulated there.

lib/ProfileData/InstrProfWriter.cpp
118

Good point. Not needed.

slingn updated this revision to Diff 40978.Nov 23 2015, 2:17 PM

Updated to use new version of SaturatingAdd() that sets if result overflow.

davidxl added inline comments.Nov 29 2015, 9:43 AM
include/llvm/ProfileData/InstrProf.h
428

Should this be Result != success?

slingn updated this revision to Diff 41643.Dec 2 2015, 9:47 AM

Updated for davidxl's comments.

include/llvm/ProfileData/InstrProf.h
428

The idea here was to give the first error to occur priority. But really the ordering of multiple errors during merge isn't critical - or even obviously better either way. So the simpler check wins.

davidxl accepted this revision.Dec 2 2015, 9:59 AM
davidxl edited edge metadata.
davidxl added inline comments.
include/llvm/ProfileData/InstrProf.h
428

Ah -- I misread the code -- but it indeed was confusing -- the new way is better.

This revision is now accepted and ready to land.Dec 2 2015, 9:59 AM
This revision was automatically updated to reflect the committed changes.