[PATCH] D126833: [SampleProfile][Inline] Annotate sample profile inline remarks with link phase (prelink/postlink) information.

[Kazu Hirata via Phabricator via llvm-commits]

kazu added a comment.
Herald added a subscriber: mtrofin.

The patch looks good in general, but I have some nit-picky suggestions.



================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:208
   const Optional<InlineContext> IC;
+  const std::string annotatedInlinePassName;
   std::unique_ptr<ImportedFunctionsInliningStatistics> ImportedFunctionsStats;
----------------
Capitalize the variable name like `AnnotatedInlinePassName`.


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:579-580
 const char *InlineAdvisor::getAnnotatedInlinePassName() {
   if (!IC.hasValue())
     return DEBUG_TYPE;
 
----------------
You should be able to remove this `if` statement.  When `IC.hasValue()` is `false`, the constructor sets `annotatedInlinePassName` to `DEBUG_TYPE`.

Once the function has been simplified down to:

```
const char *InlineAdvisor::getAnnotatedInlinePassName() {
  return annotatedInlinePassName.c_str();
}
```

you might as well move it to `InlineAdvisor.h`.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:503
+  const ThinOrFullLTOPhase LTOPhase;
+  const std::string annotatedPassName;
 
----------------
Capitalize the variable name like `AnnotatedPassName`.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:545
+private:
+  inline const char *getAnnotatedRemarkPassName() const;
 };
----------------
Remove `inline` unless you absolutely need it.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:596
 
+inline const char *SampleProfileLoader::getAnnotatedRemarkPassName() const {
+  if (!AnnotateSampleProfileInlinePhase)
----------------
Likewise, remove `inline` unless you absolutely need it.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:597-600
+  if (!AnnotateSampleProfileInlinePhase)
+    return CSINLINE_DEBUG;
+
+  return annotatedPassName.c_str();
----------------
Could you fully compute `annotatedPassName` in the constructor as you do in `InlineAdvisor.cpp`?  I'm suggesting this partly for consistency with `InlineAdvisor.cpp` and partly for readability.

Again, you might consider moving the function to the class declaration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126833