[PATCH] D129009: [LTO] Fix LTO for aliased IFuncs

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

OK, I think the approach is fine.



================
Comment at: llvm/include/llvm/IR/GlobalIFunc.h:97
+
+  /// Method to apply specific operation to all resolver-related values.
+  /// If resolver target is already a global object, then apply the operation to
----------------
Delete `Method to`


================
Comment at: llvm/include/llvm/IR/GlobalIFunc.h:99
+  /// If resolver target is already a global object, then apply the operation to
+  /// it directly. If target is an expr or an alias, evaluate it to its base
+  /// object and apply the operation for the base object and all aliases along
----------------
expr -> ConstantExpr


================
Comment at: llvm/include/llvm/IR/GlobalIFunc.h:103
+  void applyAlongResolverPath(
+      const std::function<void(const GlobalValue *)> &Op) const;
 };
----------------
Use function_ref instead.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:652
+  auto *Aliasee = A.getAliaseeObject();
+
+  // Skip summary for indirect function aliases as summary for aliasee will not
----------------
Delete the blank line and move the comment above `GlobalObject *Aliasee = A.getAliaseeObject();` (expand `auto`s which may hinder readability).


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:821
+  for (const GlobalIFunc &I : M.ifuncs()) {
+    std::function<void(const GlobalValue *)> Setter =
+        [&Index](const GlobalValue *GV) {
----------------
`I.applyAlongResolverPath([&Index](const GlobalValue *GV) { ... })`


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4106
     auto *Aliasee = A.getAliaseeObject();
-    if (!Aliasee->hasName())
-      // Nameless function don't have an entry in the summary, skip it.
+    // IFunc function and Nameless function don't have an entry in the
+    // summary, skip it.
----------------
// Skip ifunc and nameless functions which don't have an entry in the summary


================
Comment at: llvm/lib/IR/Globals.cpp:360
   DenseSet<const GlobalAlias *> Aliases;
-  return findBaseObject(this, Aliases);
+  return findBaseObject(this, Aliases, [](...) {});
 }
----------------
Change `...` to `const GlobalValue *`


================
Comment at: llvm/lib/IR/Globals.cpp:553
   DenseSet<const GlobalAlias *> Aliases;
-  return findBaseObject(getOperand(0), Aliases);
+  return findBaseObject(getOperand(0), Aliases, [](...) {});
 }
----------------
Change `...` to `const GlobalValue *`


================
Comment at: llvm/lib/IR/Globals.cpp:590
+void GlobalIFunc::applyAlongResolverPath(
+    const std::function<void(const GlobalValue *)> &Op) const {
+  DenseSet<const GlobalAlias *> Aliases;
----------------
Use `function_ref` instead.


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:1155
+        (isa<GlobalAlias>(&GV) &&
+         isa<GlobalIFunc>(dyn_cast<GlobalAlias>(&GV)->getAliaseeObject())))
+      return true;
----------------
`s/dyn_cast/cast` if known to be of the type.

`cast<GlobalAlias>(&GV).getAliaseeObject()`


================
Comment at: llvm/lib/Transforms/Utils/FunctionImportUtils.cpp:39
+
+  // ifuncs and ifunc aliasee does not have summary
+  if (isa<GlobalIFunc>(SGV) ||
----------------



================
Comment at: llvm/test/LTO/Resolution/X86/alias-indirect-function-lto.ll:1
+; RUN: opt -module-summary -o %t.bc %s
+; RUN: llvm-lto2 run %t.bc -r %t.bc,foo,px -r %t.bc,bar,px -r %t.bc,baz,px -r %t.bc,qux,px -r %t.bc,grault,px -o %t2
----------------
llvm/test/ThinLTO/X86/alias-ifunc is better.

I'll remove `-lto` from the filename since it conveys no additional information.

Additionally test `llvm-dis < %t.bc`

Test something like

```
^1 = gv: (name: "quux", summaries: (alias: (module: ^0, flags: (linkage: internal, visibility: default, notEligibleToImport: 0, live: 1, dsoLocal: 1, canAutoHide: 0), aliasee: ^6)))
^2 = ...   # have the line to test that there is no summary
^3 = ...
^6 = gv: (name: "foo_resolver", summaries: (function: (module: ^0, flags: (linkage: internal, visibility: default, notEligibleToImport: 0, live: 1, dsoLocal: 1, canAutoHide: 0), insts: 1))) 
```


================
Comment at: llvm/test/LTO/Resolution/X86/alias-indirect-function-lto.ll:31
+
+; no crash
----------------
Delete


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129009