comment changes and logic updates in TOCRestoreNeededForCallToImplementation

Author: Wael Yehia

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

@@ -3391,14 +3391,17 @@ static bool TOCRestoreNeededForCallToImplementation(const GlobalIFunc &GI) {
       return false;
     auto *F = cast<Function>(V);
 
-    // static functions are local
+    // Static functions are local
     if (F->getLinkage() == GlobalValue::InternalLinkage)
       return true;
-    // for now, declarations we treat as potentially non-local
+    // We treat declarations as non-local because the visibility attribute
+    // on a declaration might not match the definition, and AIX linker
+    // ignores the visibility on a reference.
     if (F->isDeclarationForLinker())
       return false;
-    // hidden visibility definitions cannot be preempted, so treat as local.
-    if (F->getVisibility() == GlobalValue::HiddenVisibility)
+    // hidden or protected visibility definitions cannot be preempted.
+    if (F->getVisibility() == GlobalValue::HiddenVisibility ||
+        F->getVisibility() == GlobalValue::ProtectedVisibility)
       return true;
 
     return false;
@@ -3417,9 +3420,12 @@ static bool TOCRestoreNeededForCallToImplementation(const GlobalIFunc &GI) {
       return false;
   }
 
-  // if one of the return values of the resolver function is not a
-  // local function, then we have to conservatively do a TOC save/restore.
   auto *Resolver = GI.getResolverFunction();
+  // If the resolver is preemptible then we cannot rely on its implementation.
+  if (!isLocalFunc(Resolver))
+    return true;
+  // If one of the return values of the resolver function is not a
+  // local function, then we have to conservatively do a TOC save/restore.
   for (auto &BB : *Resolver) {
     if (auto *Ret = dyn_cast<ReturnInst>(BB.getTerminator())) {
       Value *RV = Ret->getReturnValue();
@@ -3459,12 +3465,12 @@ static bool TOCRestoreNeededForCallToImplementation(const GlobalIFunc &GI) {
  *   .vbyte  4, 0
  *   .csect .foo[PR],5
  *   .ref ifunc_sec.foo[RW]
- *   ld 12, L..foo_desc(2)                   # load foo's descriptor address
- *   ld 11, 16(12)                           # load the env pointer if target might be a non-C/C++ function, otherwise this load is omitted
- *   ld 12, 0(12)                            # load foo.addr
+ *   lwz 12, L..foo_desc(2)                  # load foo's descriptor address
+ *   lwz 11, 8(12)                           # load the env pointer if target might be a non-C/C++ function
+ *   lwz 12, 0(12)                           # load foo.addr
  *   mtctr 12
  *   bctr                                    # branch to CR without setting LR so that callee returns to the caller of .foo
- *                                              # -- End function
+ *                                           # -- End function
  */
 void PPCAIXAsmPrinter::emitGlobalIFunc(Module &M, const GlobalIFunc &GI) {
   // Set the Subtarget to that of the resolver.

llvm/lib/Target/PowerPC/PPCPrepareIFuncsOnAIX.cpp

@@ -51,17 +51,18 @@ ModulePass *llvm::createPPCPrepareIFuncsOnAIXPass() {
   return new PPCPrepareIFuncsOnAIX();
 }
 
-// @foo = ifunc i32 (), ptr @foo_resolver, !associated !0
-// define ptr @foo_resolver() {
-//  ...
+// For each ifunc `foo` with a resolver `foo_resolver`, create a global variable
+// `__update_foo` in the `ifunc_sec` section, representing the pair:
+//   { ptr @foo, ptr @foo_resolver }
+// The compiler arranges for the constructor function `__init_ifuncs` to be
+// included on the link step. The constructor walks the `ifunc_sec` section,
+// calling the resolver function and storing the result in foo's descriptor.
+// On AIX, the address of a function is the address of its descriptor, so the
+// constructor accesses foo's descriptor from the first field of the pair.
 //
-// %struct.IFUNC_PAIR = type { ptr, ptr }
-// @update_foo = internal global %struct.IFUNC_PAIR { ptr @foo, ptr
-// @foo_resolver }, section "ifunc_sec", align 8, !associated !1 declare void
-// @__init_ifuncs(...)
+// Since the global `__update_foo` is unreferenced, it's liveness needs to be
+// associated to the liveness of ifunc `foo`
 //
-// !0 = !{ptr @update_foo}
-// !1 = !{ptr @__init_ifuncs}
 bool PPCPrepareIFuncsOnAIX::runOnModule(Module &M) {
   if (M.ifuncs().empty())
     return false;