[PATCH] D92493: [IR] Add hot to function attributes and use hot/cold attribute in function section prefix/suffix

[Rong Xu via Phabricator via llvm-commits]

xur marked 4 inline comments as done.
xur added inline comments.


================
Comment at: clang/test/CodeGen/attributes.c:66
 
+// CHECK: define void @t82() [[HOTDEF:#[0-9]+]] {
+void t81(void) __attribute__((hot));
----------------
MaskRay wrote:
> Nit: use `[[#HOTDEF:]]` (which is shorthand for `[[#%u,HOTDEF:]]`
Thanks. Good to know this format.


================
Comment at: llvm/docs/LangRef.rst:1506
+    When profile feedback is enabled, this attribute has the precedence over
+    the profile inforatmion. By marking a function ``hot``, users can work
+    around the cases where the training input does not have good coverage
----------------
davidxl wrote:
> typo: information. 
Fixed


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:480
       F.setSectionPrefix("hot");
-    else if (PSI->isFunctionColdInCallGraph(&F, *BFI))
+    else if (PSI->isFunctionColdInCallGraph(&F, *BFI) ||
+             F.hasFnAttribute(Attribute::Cold))
----------------
davidxl wrote:
> For conservative behavior, this should be &&
Should not be && -- this would require all cold functions be marked as cold by the users to place into unlikely section. I think it's too much.

I used '||" for the following reasons:
(1) If a function is not annotated as "cold", use PSI to determine if to place into unlikely section. This is no change from current behavior.
(2) If the function is marked as code
(2.1)  if PSI shows the function is HOT, place into 'hot' section
(2.2)  if PSI shows cold, place into 'unlikely' section
(2.3)  if PSI shows neither code nor hot. This is a warm function. We honor the annotation, and place into 'unlikely" section.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1758
+    // attribute, user's annotation has the precedence over the profile.
+    if (F->hasFnAttribute(Attribute::Hot))
+      continue;
----------------
davidxl wrote:
> Perhaps emit a warning here?
Added.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92493/new/

https://reviews.llvm.org/D92493