[PATCH] D106448: [llvm][Inline] Add a module level inliner

[Kazu Hirata via Phabricator via llvm-commits]

kazu added a comment.

Your patch is looking good.  Would you mind addressing mostly cosmetic comments?  Thanks!



================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:895-906
+    InlineParams IP = getInlineParamsFromOptLevel(Level);
+    if (Phase == ThinOrFullLTOPhase::ThinLTOPreLink && PGOOpt &&
+        PGOOpt->Action == PGOOptions::SampleUse)
+      IP.HotCallSiteThreshold = 0;
+
+    if (PGOOpt)
+      IP.EnableDeferral = EnablePGOInlineDeferral;
----------------
May I suggest you to put this in a function so that you can do something similar to the else clause just a couple of lines below?

```
  MPM.addPass(buildInlinerPipeline(Level, Phase));
```

Maybe you can create a function named `buildModuleInlinerPipeline` or something.

This way, the details specific to the module inliner don't clutter `buildModuleSimplificationPipeline` as much.



================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:903-904
+
+    if (EnableModuleInliner)
+      IP.EnableDeferral = false;
+
----------------
Does your module inliner honor `EnableDeferral` at all?  If so, and there is a good reason to disable it, please add an appropriate comment.

In any event, you should be able to remove `if (EnableModuleInliner)` because we are already inside `if (EnableModuleInliner)` above.


================
Comment at: llvm/lib/Transforms/IPO/ModuleInliner.cpp:368
+}
\ No newline at end of file

----------------
Please add a new line at the end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106448