Page MenuHomePhabricator

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

Authored by stefanp on Jan 3 2019, 12:10 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

stefanp created this revision.Jan 3 2019, 12:10 PM
Hahnfeld added a subscriber: Hahnfeld.

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

stefanp updated this revision to Diff 180133.Jan 3 2019, 1:33 PM

Modified patch to completely revert rL349512.

Gentle ping..

hfinkel accepted this revision.Feb 25 2019, 10:26 AM

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

This revision is now accepted and ready to land.Feb 25 2019, 10:26 AM

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.

@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.

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?

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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2019, 1:19 PM