[PATCH] D24989: Use profile info to set function section prefix to group hot/cold functions.

[Easwaran Raman via llvm-commits]

eraman added a comment.

I've some minor comments, but otherwise LGTM.



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:216
+                         "Optimize for code generation", false, false)
+INITIALIZE_PASS_DEPENDENCY(ProfileSummaryInfoWrapperPass)
+INITIALIZE_TM_PASS_END(CodeGenPrepare, "codegenprepare",
----------------
This is not part of your change, but why are the other analysis passes (LoopInfoWrapper etc) already specified in INITIALIZE_PASS_DEPENDENCY? 


================
Comment at: lib/IR/Function.cpp:1280
+  MDBuilder MDB(getContext());
+  setMetadata(LLVMContext::MD_section_prefix, MDB.createFunctionSectionPrefix(Prefix));
+}
----------------
This seems to exceed 80 char limit. Did you run clang-format?


================
Comment at: lib/IR/Function.cpp:1285
+  MDNode *MD = getMetadata(LLVMContext::MD_section_prefix);
+  if (MD && MD->getOperand(0))
+    if (MDString *MDS = dyn_cast<MDString>(MD->getOperand(0)))
----------------
if MD is non nullptr, then you shouldn't have to check for MD->getOperand(0) to be non-null right? Similarly, below, you don't have to check that operand 0 equals "function_section_prefix". Removing the ifs will make this clean. Or may be add asserts?


https://reviews.llvm.org/D24989