[OPENMP] Deal with additional store inserted by Clang under -fno-PIC for PowerPC.

[stefanp]

Summary

Changing the default from -fPIC to -fno-PIC on PowerPC exposed an issue in OpenMP for PowerPC.
The issue is reported here:
https://bugs.llvm.org/show_bug.cgi?id=40082

This is a fix for that issue.
Also removed the XFAIL from the two tests that were failing under -fno-PIC.

[Hahnfeld]

I think you should also revert the other changes from rL349512.

[stefanp]

Modified patch to completely revert rL349512.

[stefanp]

Gentle ping..

[stefanp]

Ping.

[hfinkel]

Is this STW instruction something that instruction scheduling might move? (@nemanjai ?)

[nemanjai]

In D56286#1409384, @hfinkel wrote:

Is this STW instruction something that instruction scheduling might move? (@nemanjai ?)

I am fairly certain that the STW is the store of the value returned by the function so the scheduler should leave it alone. Perhaps @stefanp can verify this.

[stefanp]

@hfinkel @nemanjai
Hi,
Sorry for the late reply. It took me a little while to figure out that I had specified the wrong options for my test when trying to reproduce this....
Anyway, I've verified that what Nemanja said is indeed correct:
no-PIC

bl      100006b0 <00000017.plt_call.omp_control_tool@@VERSION>                                       
ld      r2,24(r1)                                                                                    
nop                                                                                                  
stw     r3,108(r31)
nop

PIC

bl      100006b0 <00000017.plt_call.omp_control_tool@@VERSION>
ld      r2,24(r1)
nop
nop

The store in the no-PIC version is just saving the return value from r3.

[Hahnfeld]

In D56286#1411846, @nemanjai wrote:

In D56286#1409384, @hfinkel wrote:

Is this STW instruction something that instruction scheduling might move? (@nemanjai ?)

I am fairly certain that the STW is the store of the value returned by the function so the scheduler should leave it alone. Perhaps @stefanp can verify this.

Well yes, but the return value is never used. So why does the no-PIC version need to store it while the PIC version omits that instruction?

[stefanp]

The reason why we don't clean up that extra store on no-PIC is because we run a couple of extra passes for PIC for PowerPC.
It is this code:

// FIXME: We probably don't need to run these for -fPIE.
if (getPPCTargetMachine().isPositionIndependent()) {
  // FIXME: LiveVariables should not be necessary here!
  // PPCTLSDynamicCallPass uses LiveIntervals which previously dependent on
  // LiveVariables. This (unnecessary) dependency has been removed now,
  // however a stage-2 clang build fails without LiveVariables computed here.
  addPass(&LiveVariablesID, false);
  addPass(createPPCTLSDynamicCallPass());
}

Those extra passes clean up the extra store.
If I remember correctly this is only really an issue at -O0. At higher opts other passes clean up stores like that.

openmp/runtime/test/ompt/callback.h

 #if KMP_ARCH_X86 || KMP_ARCH_X86_64
 // On X86 the NOP instruction is 1 byte long. In addition, the comiler inserts
 // a MOV instruction for non-void runtime functions which is 3 bytes long.
 #define print_possible_return_addresses(addr) \
   printf("%" PRIu64 ": current_address=%p or %p for non-void functions\n", \
          ompt_get_thread_data()->value, ((char *)addr) - 1, ((char *)addr) - 4)
 #elif KMP_ARCH_PPC64
 // On Power the NOP instruction is 4 bytes long. In addition, the compiler
-// inserts an LD instruction which accounts for another 4 bytes. In contrast to
-// X86 this instruction is always there, even for void runtime functions.
+// inserts a second NOP instruction (another 4 bytes). For non-void runtime
+// functions Clang inserts a STW instruction (but only if compiling under
+// -fno-PIC which will be the default with Clang 8.0, another 4 bytes).
 #define print_possible_return_addresses(addr) \
-  printf("%" PRIu64 ": current_address=%p\n", ompt_get_thread_data()->value, \
-         ((char *)addr) - 8)
+  printf("%" PRIu64 ": current_address=%p or %p\n", ompt_get_thread_data()->value, \
+         ((char *)addr) - 8, ((char *)addr) - 12)
 #elif KMP_ARCH_AARCH64
 // On AArch64 the NOP instruction is 4 bytes long, can be followed by inserted
 // store instruction (another 4 bytes long).
 #define print_possible_return_addresses(addr) \
   printf("%" PRIu64 ": current_address=%p or %p\n", ompt_get_thread_data()->value, \
          ((char *)addr) - 4, ((char *)addr) - 8)
 #else
 #error Unsupported target architecture, cannot determine address offset!

openmp/runtime/test/ompt/misc/control_tool.c

 // RUN: %libomp-compile-and-run | FileCheck %s
 // REQUIRES: ompt
 // UNSUPPORTED: gcc-4, gcc-5, gcc-6, gcc-7
-// XFAIL: powerpc64le, ppc64le
 #define TEST_NEED_PRINT_FRAME_FROM_OUTLINED_FN
 #include "callback.h"
 #include <omp.h>
 
 int main()
 {
   #pragma omp parallel num_threads(1)
   {

openmp/runtime/test/ompt/synchronization/taskwait.c

 // RUN: %libomp-compile-and-run | %sort-threads | FileCheck %s
 // REQUIRES: ompt
 // UNSUPPORTED: gcc-4, gcc-5, gcc-6, gcc-7
-// XFAIL: powerpc64le, ppc64le
 #include "callback.h"
 #include <omp.h>
 
 int main()
 {
   int x = 0;
   #pragma omp parallel num_threads(2)
   {