[PATCH] D84167: [Attributor] Use internalized version of non-exact functions

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:96
+                     cl::desc("Allow the Attributor to use IP information "
+                              "derived from non-exact functions"),
+                     cl::init(false));
----------------
maybe state that this is done via cloning, or at least that this is sound.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1424
+/// definitions with the same name have the same semantics as this one
+///
+static Function *internalizeFunction(Function &F) {
----------------
Typo above.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1438
+      Function::Create(FnTy, llvm::GlobalValue::InternalLinkage,
+                       F.getAddressSpace(), F.getName() + "_copied");
+  ValueToValueMapTy VMap;
----------------
Maybe: ".internalized" ?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1449
+  // Copy the body of the original function to the new one
+  llvm::CloneFunctionInto(Copied, &F, VMap, /* ModuleLevelChanges */ true,
+                          Returns);
----------------
fhahn wrote:
> nit: we are already using the llvm namespace, there should be no need to use `llvm::`. same at other places in the patch,
Are there really ModuleLevelChanges?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2199
+          if (const auto *CB = dyn_cast<CallBase>(U.getUser())) {
+            auto *CallerF = const_cast<Function *>(CB->getCaller());
+            CGUpdater.reanalyzeFunction(*CallerF);
----------------
Remove the const above so you can remove the const_cast.


================
Comment at: llvm/test/Transforms/Attributor/internalize.ll:1
+; RUN: opt -attributor -attributor-cgscc -disable-inlining -attributor-allow-deep-wrappers -S < %s | FileCheck %s --check-prefix=DWRAPPER
+
----------------
Please add all 4 run lines we usually use, once with and once without the flag.
We can also use the update script then. We soon get the ability to check for new functions but for now you can use some manual lines as you did so far for those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84167