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

[Easwaran Raman via llvm-commits]

eraman added inline comments.

================
Comment at: include/llvm/ProfileData/ProfileCommon.h:32
@@ -30,1 +31,3 @@
 struct InstrProfRecord;
+inline const char *getHotSectionPrefix() { return ".hot"; }
+inline const char *getUnlikelySectionPrefix() { return ".unlikely"; }
----------------
davidxl wrote:
> eraman wrote:
> > These two functions should be elsewhere. Perhaps in TargetLowering ?
> I think it is fine to be here -- it is generic enough and it is profile related.
The strings themselves are not profile related. One might want to use .unlikely for error handling routines even without profile data.

================
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:
> > > 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). 


http://reviews.llvm.org/D17460