[PATCH] D17460: Add prefix based function layout when profile is available.

[Dehao Chen via llvm-commits]

danielcdh added inline comments.

================
Comment at: lib/ProfileData/ProfileSummary.cpp:80
@@ -77,1 +79,3 @@
 
+bool ProfileSummary::hasProfile() {
+  // FIXME: update when summary data is stored in module's metadata.
----------------
davidxl wrote:
> If this takes Function * as an argument, it can be a wrapper of getEntryCount call
!getEntryCount() does not mean no profile for the binary, but no profile for the function. We need to have this function to indicate if this is an FDO/AFDO build.

================
Comment at: lib/ProfileData/ProfileSummary.cpp:91
@@ +90,3 @@
+
+bool ProfileSummary::isFunctionUnlikely(const Function *F) {
+  if (F->hasFnAttribute(Attribute::Cold)) {
----------------
davidxl wrote:
> eraman wrote:
> > davidxl wrote:
> > > eraman wrote:
> > > > davidxl wrote:
> > > > > I suggest changing this interface's name to be 'isFunctionRarelyCalled'. It is different from 'isFunctionCold' which should be checking the internal block count. These two interfaces are for different purposes. The later can be used to guide decisions such as optimize for size etc, while the former can be used for layout decision to reduce itlb misses.
> > > > > 
> > > > > Similarly, isFunctionHot does not mean it is frequently called either -- so the interface need to be split up too.
> > > > This and isFunctionHot should be made as a member functions of  Function. 
> > > I think it is better to keep all/most of the profile related query APIs here in this file.
> > Again, the function itself is not fully profile dependent. I think ProfileCommon should only have APIs like isCountConsideredHot(uint64_t Count). 
> You have a point about Function being not profile dependent, but I think we should not restrict the scope of ProfileCommon.h too much like this. It is reasonable for ProfileCommon to reference IR related headers.
> 
> 
After a second thought, I think it still makes sense to group "hot" functions together instead of "frequently called" This is because the hot path of an "infrequently called" hot function may have many calls to a frequently called function. So if these two functions are far apart, it will waste an ITLB entry. Additionally, this will pollute the heat map and makes performance tuning more difficult.

================
Comment at: lib/ProfileData/ProfileSummary.cpp:95
@@ +94,3 @@
+  }
+  if (!hasProfile() || !F->getEntryCount()) {
+    return false;
----------------
eraman wrote:
> davidxl wrote:
> > the check of hasProfile is probably redundant.
> I think what David says is true (!F->genEntryCount() implies !hasProfile()). 
I've updated the logic, PTAL.


http://reviews.llvm.org/D17460