This is an archive of the discontinued LLVM Phabricator instance.

[PGO] assignment operator does not get profile data
ClosedPublic

Authored by davidxl on Feb 6 2016, 11:57 AM.

Details

Summary

For compiler generated assignment operator that is not trivial (calling base class operator=()), Clang FE assign region counters to the function body but does not emit profile counter increment for the function entry. This leads to many problems:

  1. the operator body does not have profile data generated leading to warning in profile-use
  2. the size of the function body may be large and lack of profile data may lead to wrong inlining decisions
  3. when FE assign region counters to the function, it also emit coverage mapping data for the function -- but it has no coverage data which is confusing (currently the llvm-cov tool will report malformed format (as the name of the operator is not put into the right name section).

Diff Detail

Repository
rL LLVM

Event Timeline

davidxl updated this revision to Diff 47089.Feb 6 2016, 11:57 AM
davidxl retitled this revision from to [PGO] assignment operator does not get profile data.
davidxl updated this object.
davidxl added a reviewer: vsk.
davidxl added subscribers: llvm-commits, cfe-commits.
davidxl updated this revision to Diff 47150.Feb 7 2016, 12:19 PM

Updated test case.

Another test case in profile-rt (pending) is : http://reviews.llvm.org/D16974

vsk edited edge metadata.Feb 8 2016, 8:46 AM

Thanks! This is definitely the right fix, since it fixes up implicitly-generated move constructors as well. However, I think the test can be simpler (while also checking that move constructors are handled correctly). E.g:

struct A {
  void operator=(const A &a) {}
  void operator=(const A &&a) {}
};
struct B {
  A a;
  B &operator=(const B &b) = default;
  B &operator=(B &&b) = default;
};
int main() {
  B b1, b2;
  b1 = b2;
  b2 = static_cast<B &&>(b1);
  return 0;
}
davidxl updated this revision to Diff 47217.Feb 8 2016, 9:25 AM
davidxl edited edge metadata.

Simplified test case suggested by Vedant.

This looks like a change to clang - could you test it in clang (& review it
on cfe-commits instead of llvm-commits)?

davidxl updated this revision to Diff 47239.Feb 8 2016, 12:18 PM

Further simplify tests according to David B's comment.

This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.Feb 9 2016, 12:53 PM
cfe/trunk/test/Profile/def-assignop.cpp
27

This doesn't need to be main or have an int return. Just make it a void function (with some generic name) & drop the "return 0" to keep it simple.