[PATCH] D150148: [IPO] Opt-in local clones for thinlto imports

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Just a few minor comments/suggestions



================
Comment at: llvm/lib/Transforms/IPO/ElimAvailExtern.cpp:49
+/// upstream the pipeline (e.g. ICP).
+static void handleThinltoCopy(Function &F) {
+  assert(F.hasAvailableExternallyLinkage());
----------------
In theory we could have a non-thinlto available_externally function, so maybe rename this to something more generic like convertToCopy.


================
Comment at: llvm/lib/Transforms/IPO/ElimAvailExtern.cpp:61
+  auto OrigName = F.getName().str();
+  auto *M = F.getParent();
+  assert(M);
----------------
Maybe just pass this in from the caller that already has it.


================
Comment at: llvm/lib/Transforms/IPO/ElimAvailExtern.cpp:70
+  F.setName(NewName);
+  if (auto *SP = F.getSubprogram()) {
+    SP->replaceLinkageName(MDString::get(F.getParent()->getContext(), NewName));
----------------
Nit: remove braces


================
Comment at: llvm/lib/Transforms/IPO/ElimAvailExtern.cpp:73
+  }
+  // Making the function
+  F.setLinkage(GlobalValue::InternalLinkage);
----------------
Incomplete comment?


================
Comment at: llvm/lib/Transforms/IPO/ElimAvailExtern.cpp:112
+
+    if (ConvertToLocal) {
+      handleThinltoCopy(F);
----------------
Nit remove braces around single statement if / else bodies


================
Comment at: llvm/lib/Transforms/IPO/ElimAvailExtern.cpp:120
     F.removeDeadConstantUsers();
     NumFunctions++;
     Changed = true;
----------------
This tracks the removed functions (per string in STATISTIC). Maybe add a second one for tracking the number of conversions? If you do so, add -stats to your new test and check it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150148