This is an archive of the discontinued LLVM Phabricator instance.

Tolerate negative offset when matching sample profile.
ClosedPublic

Authored by danielcdh on Oct 20 2015, 2:24 PM.

Details

Reviewers
dnovillo
davidxl
Summary

In some cases (as illustrated in the unittest), lineno can be less than the heade_lineno because the function body are included from some other files. In this case, offset will be negative. This patch makes clang still able to match the profile to IR in this situation.

Diff Detail

Event Timeline

danielcdh updated this revision to Diff 37921.Oct 20 2015, 2:24 PM
danielcdh retitled this revision from to Tolerate negative offset when matching sample profile..
danielcdh updated this object.
danielcdh added reviewers: dnovillo, davidxl.
danielcdh added a subscriber: llvm-commits.
dnovillo added inline comments.Oct 20 2015, 2:42 PM
lib/Transforms/IPO/SampleProfile.cpp
245–246

Could you factor out this offset computation and add a comment? It's not immediately obvious why the code is doing this saturation and it's used in several places. Also, what's guaranteeing that the saturation will be done on 16 bits?

danielcdh updated this revision to Diff 37940.Oct 20 2015, 3:26 PM

Refactor.

danielcdh added inline comments.Oct 20 2015, 3:31 PM
lib/Transforms/IPO/SampleProfile.cpp
245–246

Done.

In create_llvm_prof, offset is only encoded in 16 bits.

dnovillo accepted this revision.Oct 20 2015, 3:56 PM
dnovillo edited edge metadata.

LGTM with the additional check. Thanks.

lib/Transforms/IPO/SampleProfile.cpp
186

I would now assert that this is the case (ie, L - H fits in 16 bits). It's true that most input will come straight out of create_llvm_prof, but not all of it will. In particular, the offsets in the input profiles are specified to be 32-bit unsigned integers.

Alternatively, we could assert this in ProfileData/SampleProfReader.cpp itself. This needs to be done in all 3 readers. Perhaps that's a better place to assert this.

245–246

Sure, but we're sufficiently removed from create_llvm_prof here that it makes sense to remind the user :)

This revision is now accepted and ready to land.Oct 20 2015, 3:56 PM
danielcdh updated this revision to Diff 37943.Oct 20 2015, 4:24 PM
danielcdh edited edge metadata.

Add sanity check.

danielcdh added inline comments.Oct 20 2015, 4:26 PM
lib/Transforms/IPO/SampleProfile.cpp
186

Added sanity check for LineLocation in text and binary reader. For gcov reader, the reader itself use bit operation to set LineLocation, which ensures that it's always within 16 bits.

danielcdh updated this revision to Diff 38747.Oct 29 2015, 10:31 AM

Update the discriminator assignment algorithm.

  • If a scope has already been assigned a discriminator, do not reassign a nested discriminator for it.
  • If the file and line both match, even if the column does not match, we should assign a new discriminator for the stmt.

supposed to create a new patch... please ignore the previous update sent.

new patch created in http://reviews.llvm.org/D14181

danielcdh closed this revision.Oct 29 2015, 2:47 PM