[PowerPC] Fix missing nop after call to weak callee. (llvm 10.x)

[abebeos]

Summary

Authored by stefanp on Tue, Dec 8, 6:57 AM.

Original Patch: https://reviews.llvm.org/D91983

Backported to llvm-10.x, in order to remove dormant bug within:

https://github.com/numba/numba/issues/6606

llvm-testsuite on Power8 HW went fine.

(meta: Phabricator revisions are quite fine. But I have no idea how to select a target-branch, which would be 10.x, to see some builds&tests...)

[nemanjai]

@abebeos Unfortunately there is no method for us to backport this to LLVM 10.x. Older releases are not maintained. There is typically a single point release for bug fixes after every major release and we have unfortunately missed the deadline for 11.0.1 with this so it will only be in 12.0.0.
Is it possible to maintain this patch as a necessary fix for LLVM 10 in Numba?

[abebeos]

In D94183#2491082, @nemanjai wrote:

@abebeos Unfortunately there is no method for us to backport this to LLVM 10.x. Older releases are not maintained. There is typically a single point release for bug fixes after every major release and we have unfortunately missed the deadline for 11.0.1 with this so it will only be in 12.0.0.

Oh, I see!

Is it possible to maintain this patch as a necessary fix for LLVM 10 in Numba?

Yes I guess, I created already a PR: https://github.com/numba/llvmlite/pull/684

The other problem I have is that I still need a confirmation that the patch is valid (I validated with llvm and numba testsuites on PowerPC HW, but have no confirmation from an insider or a public testsuite).

btw: edelsohn@github asked for your feedback here: btw: https://github.com/numba/numba/issues/4026#issuecomment-752241417

[abebeos]

In D94183#2491284, @abebeos wrote:

In D94183#2491082, @nemanjai wrote:

@abebeos Unfortunately there is no method for us to backport this to LLVM 10.x. Older releases are not maintained. There is typically a single point release for bug fixes after every major release and we have unfortunately missed the deadline for 11.0.1 with this so it will only be in 12.0.0.

[...]

The other problem I have is that I still need a confirmation that the patch is valid (I validated with llvm and numba testsuites on PowerPC HW, but have no confirmation from an insider or a public testsuite).

Essentially, the patch is minimal anyways, kind of "keep it safer, at the possible cost of a redundant nop).

In any way, I understand that this can be closed as "can't fix" / "won't fix" (did not find a way to close it myself).

[abebeos]

Patch not applicable, closing

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

    if (!G)
      return false;
 
-   const GlobalValue *GV = G->getGlobal();
+
+  // If the GV is not a strong definition then we need to assume it can be
+  // replaced by another function at link time. The function that replaces
+  // it may not share the same TOC as the caller since the callee may be
+  // replaced by a PC Relative version of the same function.
+  if (!GV->isStrongDefinitionForLinker())
+    return false;
+
   // The medium and large code models are expected to provide a sufficiently
   // large TOC to provide all data addressing needs of a module with a
   // single TOC. Since each module will be addressed with a single TOC then we

       CodeModel::Large == TM.getCodeModel())
     return TM.shouldAssumeDSOLocal(*Caller->getParent(), GV);
 
-  // Otherwise we need to ensure callee and caller are in the same section,
-  // since the linker may allocate multiple TOCs, and we don't know which
-  // sections will belong to the same TOC base.
-
-  if (!GV->isStrongDefinitionForLinker())
-    return false;
-
   // Any explicitly-specified sections and section prefixes must also match.
   // Also, if we're using -ffunction-sections, then each function is always in
   // a different section (the same is true for COMDAT functions).

llvm/test/CodeGen/PowerPC/ppc64-blnop.ll

 
 ; CHECK-LABEL: wo_hcaller:
 ; CHECK: bl wo_hcallee
-; CHECK-NOT: nop
+; CHECK-NEXT: nop
 
 ; SCM-LABEL: wo_hcaller:
 ; SCM:       bl wo_hcallee

 
 ; CHECK-LABEL: wo_pcaller:
 ; CHECK: bl wo_pcallee
-; CHECK-NOT: nop
+; CHECK-NEXT: nop
 
 ; SCM-LABEL:   wo_pcaller:
 ; SCM:         bl wo_pcallee

 
 ; CHECK-LABEL: w_pcaller:
 ; CHECK: bl w_pcallee
-; CHECK-NOT: nop
+; CHECK-NEXT: nop
 
 ; SCM-LABEL: w_pcaller:
 ; SCM:       bl w_pcallee

 
 ; CHECK-LABEL: w_hcaller:
 ; CHECK: bl w_hcallee
-; CHECK-NOT: nop
+; CHECK-NEXT: nop
 
 ; SCM-LABEL: w_hcaller:
 ; SCM:       bl w_hcallee

llvm/test/CodeGen/PowerPC/ppc64-calls.ll

   ret void
 }
 
-; Calls to weak function requires a TOC restore 'nop' with the small codemodel
+; Calls to weak function requires a TOC restore 'nop' with all code models
 ; because the definition that gets choosen at link time may come from a
-; different section even though we have seen a weak definition in the same
-; section at compile time.
-; With large and medium codemodels no TOC restore is needed, since we know
-; whichever definition is choosen it resides within the same DSO boundaries and
-; therefore shares the same TOC.
+; different compilation unit that was compiled with PC Relative and has no TOC.
 define void @test_weak() nounwind readnone {
   tail call void @foo_weak() nounwind
 ; CHECK-LABEL: test_weak:
-; CHECK: b foo_weak
-; CHECK-NOT: nop
+; CHECK:       bl foo_weak
+; CHECK-NEXT:  nop
 
 ; SCM-LABEL: test_weak:
 ; SCM:       bl foo_weak

llvm/test/CodeGen/PowerPC/ppc64-sibcall.ll

   ret void
 
 ; CHECK-SCO-LABEL: wo_hcaller:
-; CHECK-SCO: b wo_hcallee
+; CHECK-SCO: bl wo_hcallee
 
 ; SCM-LABEL: wo_hcaller:
 ; SCM:       bl wo_hcallee

   ret void
 
 ; CHECK-SCO-LABEL: wo_pcaller:
-; CHECK-SCO: b wo_pcallee
+; CHECK-SCO: bl wo_pcallee
 
 ; SCM-LABEL: wo_pcaller:
 ; SCM:       bl wo_pcallee

   ret void
 
 ; CHECK-SCO-LABEL: wo_caller:
-; CHECK-SCO: b wo_callee
+; CHECK-SCO: bl wo_callee
 
 ; SCM-LABEL: wo_caller:
 ; SCM:       bl wo_callee

   ret void
 
 ; CHECK-SCO-LABEL: w_pcaller:
-; CHECK-SCO: b w_pcallee
+; CHECK-SCO: bl w_pcallee
 
 ; SCM-LABEL: w_pcaller:
 ; SCM:       bl w_pcallee

   ret void
 
 ; CHECK-SCO-LABEL: w_hcaller:
-; CHECK-SCO: b w_hcallee
+; CHECK-SCO: bl w_hcallee
 
 ; SCM-LABEL: w_hcaller:
 ; SCM:       bl w_hcallee

   ret void
 
 ; CHECK-SCO-LABEL: w_caller:
-; CHECK-SCO: b w_callee
+; CHECK-SCO: bl w_callee
 
 ; SCM-LABEL: w_caller:
 ; SCM:       bl w_callee

llvm/test/CodeGen/PowerPC/pr41088.ll

 ; CHECK-NEXT:  # %bb.2: # %bb12
 ; CHECK-NEXT:    clrldi r4, r3, 32
 ; CHECK-NEXT:    bl test3
+; CHECK-NEXT:    nop
 ; CHECK-NEXT:    addi r1, r1, 32
 ; CHECK-NEXT:    ld r0, 16(r1)
 ; CHECK-NEXT:    mtlr r0

llvm/test/CodeGen/PowerPC/preemption.ll

 
 ; STATIC-LABEL: @weak_default_function_caller
 ; STATIC:       bl weak_default_function
-; STATIC-NOT:   nop
+; STATIC-NEXT:  nop
 ; STATIC:       blr
 
 ; CHECK-LABEL:  @weak_default_function_caller

 
 ; STATIC-LABEL: @weak_local_function_caller
 ; STATIC:       bl weak_local_function
-; STATIC-NOT:   nop
+; STATIC-NEXT:  nop
 ; STATIC:       blr
 
 ; CHECK-LABEL:  @weak_local_function_caller
 ; CHECK:        bl weak_local_function
-; CHECK-NOT:    nop
+; CHECK-NEXT:   nop
 ; CHECK:        blr
 }
 

 
 ; STATIC-LABEL: @external_local_function_caller
 ; STATIC:       bl external_local_function
-; STATIC-NOT:  nop
+; STATIC-NEXT:  nop
 ; STATIC:       blr
 
 ; CHECK-LABEL:  @external_local_function_caller
 ; CHECK:        bl external_local_function
-; CHECK-NOT:    nop
+; CHECK-NEXT:   nop
 ; CHECK:        blr
 }
 

 
 ; STATIC-LABEL: @weak_preemptable_function_caller
 ; STATIC:       bl weak_preemptable_function
-; STATIC-NOT:   nop
+; STATIC-NEXT:  nop
 ; STATIC:       blr
 
 ; CHECK-LABEL:  @weak_preemptable_function_caller

llvm/test/CodeGen/PowerPC/xray-tail-call-hidden.ll

 ; CHECK-NEXT:         nop
 ; CHECK-NEXT:         mtlr 0
 ; CHECK-LABEL: .Ltmp1:
+; CHECK:              bl callee
+; CHECK-NEXT:         nop
   %retval = tail call i32 @callee()
   ret i32 %retval
 ; CHECK-LABEL: .Ltmp2:
-; CHECK:              b callee
-; CHECK-NEXT:         nop
-; CHECK-NEXT:         std 0, -8(1)
+; CHECK:              std 0, -8(1)
 ; CHECK-NEXT:         mflr 0
 ; CHECK-NEXT:         bl __xray_FunctionExit
 ; CHECK-NEXT:         nop