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

[David Li via llvm-commits]

davidxl 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"; }
----------------
eraman wrote:
> 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.
Aren't those also related to profile too (it is profile that can be determined statically)?

================
Comment at: lib/ProfileData/ProfileSummary.cpp:91
@@ +90,3 @@
+
+bool ProfileSummary::isFunctionUnlikely(const Function *F) {
+  if (F->hasFnAttribute(Attribute::Cold)) {
----------------
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.




http://reviews.llvm.org/D17460