Only consider local functions local for the purpose of NOP eliminate after calls on PPC64

[joerg]

Summary

As shown in https://llvm.org/bugs/show_bug.cgi?id=30868 the current assumption that ld will use local calls for global symbols is incorrect. For the nop elimination, we must use the much stricter ELF local symbol definition. At least GNU ld does assume that the call is otherwise interposable.

[joerg]

Update the one user of isLocalCall to drop the now redundant PIC check.

[hfinkel]

As shown in https://llvm.org/bugs/show_bug.cgi?id=30868 the current assumption that ld will use local calls for global symbols is incorrect.

But the PR does not actually show anything. You simply assert that, "The SysV ABI on PPC64 is supposed to be implicitly PIC all the time." As I stated in D26326, Please demonstrate where the ABI requires this; I'm not sure that your premise is correct.

I really want to understand what situation actually motivated you to look at this? Was it some large binary where some long-call stub was needed?

[joerg]

http://math-atlas.sourceforge.net/devel/assembly/PPC-elf64abi-1.7.pdf

3.5.11 Function Calls

Programs use the 64-bit PowerPC bl instruction to make direct function calls. The bl instruction must be
followed by a nop instruction.

---

The definition we currently use for whether a call is interposable doesn't agree with the logic in GNU ld or GCC. While it doesn't matter for inlined functoins, a call must contain the nop as the target can be replaced.

[hfinkel]

In D26290#590529, @joerg wrote:

http://math-atlas.sourceforge.net/devel/assembly/PPC-elf64abi-1.7.pdf

3.5.11 Function Calls

Programs use the 64-bit PowerPC bl instruction to make direct function calls. The bl instruction must be
followed by a nop instruction.

---

The definition we currently use for whether a call is interposable doesn't agree with the logic in GNU ld or GCC. While it doesn't matter for inlined functoins, a call must contain the nop as the target can be replaced.

Alright; I think we're confusing several issues here. This does not have anything to do with PIC vs. non-PIC code. Yes, the bl will require a following nop so that we can patch in the TOC-restore when required. This will be necessary for shared libraries and other multi-TOC configurations.

However, I think that we really need to understand why you're running into a problem here. The range of bl is the same as b, and so you don't gain any extra range by stub creation here. The only thing I've been able to think of is that you're using -ffunction-sections and the linker is re-arranging things so that the direct branch needs a stub, and the linker requires the nop patch for the stub. If I'm right, then I'm fine with doing this when using -ffunction-sections.

[joerg]

I don't use -ffunction-sections or similar. It is a combination of two factors. The default assumption of PIC and the incorrect modelling of interposition rules in LLVM. GCC will emit calls the PLT for f on x86 when using PIC. That's the same behavior GNU ld implements for the relocation on PPC64.
It is most visible with C++ libraries.

[hfinkel]

In D26290#591010, @joerg wrote:

I don't use -ffunction-sections or similar. It is a combination of two factors. The default assumption of PIC and the incorrect modelling of interposition rules in LLVM. GCC will emit calls the PLT for f on x86 when using PIC. That's the same behavior GNU ld implements for the relocation on PPC64.
It is most visible with C++ libraries.

Can you please provide a test case which fails to link (offline if necessary)? I'm still failing to understand the underlying problem and have not run into issues myself. Thanks!

[joerg]

Superseded by D27231

lib/Target/PowerPC/PPCISelLowering.cpp

 bool isLocalCall(const SDValue &Callee)
 {
   if (GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee))
-    return G->getGlobal()->isStrongDefinitionForLinker();
+    return G->getGlobal()->hasLocalLinkage();
   return false;
 }
 

       // The address needs to go after the chain input but before the flag (or
       // any other variadic arguments).
       Ops.insert(std::next(Ops.begin()), AddTOC);
-    } else if ((CallOpc == PPCISD::CALL) &&
-               (!isLocalCall(Callee) ||
-                DAG.getTarget().getRelocationModel() == Reloc::PIC_))
+    } else if (CallOpc == PPCISD::CALL && !isLocalCall(Callee))
       // Otherwise insert NOP for non-local calls.
       CallOpc = PPCISD::CALL_NOP;
   }

test/CodeGen/PowerPC/call-nop.ll

+; RUN: llc -O2 < %s | FileCheck %s
+
+target datalayout = "E-m:e-i64:64-n32:64"
+target triple = "powerpc64--linux"
+
+@f.x = internal global i32 0, align 4
+
+; Function Attrs: noinline nounwind
+define signext i32 @f() noinline {
+  %1 = load volatile i32, i32* @f.x, align 4
+  ret i32 %1
+}
+
+; Function Attrs: nounwind
+define signext i32 @g() {
+  %1 = call signext i32 @f()
+  %2 = call signext i32 @f()
+  %3 = add nsw i32 %1, %2
+  ret i32 %3
+}
+
+; CHECK-LABEL: @g
+; CHECK:      bl f
+; CHECK-NEXT: nop
+; CHECK:      bl f
+; CHECK-NEXT: nop
+