Page MenuHomePhabricator

[PowerPC] Make no-PIC default to match GCC - LLVM
ClosedPublic

Authored by stefanp on Oct 17 2018, 12:36 PM.

Details

Summary

Ran a quick test and determined that for GCC the default option is -fno-PIC for both ELFv1 and ELFv2. We are currently using -fPIC as the default for LLVM.
Would like to revert to what GCC is doing on PowerPC.

NOTE: This patch does not yet include test case updates. Currently working on those updates and will further update this patch at that time. In the meantime I put up this patch to make sure that this is in fact what we want to do.

Diff Detail

Repository
rL LLVM

Event Timeline

stefanp created this revision.Oct 17 2018, 12:36 PM
stefanp updated this revision to Diff 170401.Oct 22 2018, 6:32 AM

Updated test cases.

stefanp updated this revision to Diff 170927.Oct 24 2018, 10:13 AM

Default on all Power PC platforms is now no-PIC.
If any bugs are exposed by this change we will fix them as they are exposed.

jsji added a comment.EditedOct 24 2018, 11:42 AM

I am seeing two choices in the testcases you updated: one is adding -relocation-model=pic, the other is to update code sequeces.
What is your general rules to choose which one to do?

For those testcase updated with "-relocation-model=pic," do we know what might happen if default is no-PIC?

I am asking because it looks like the choice is kind of random (?).
Some may need more careful thoughts?

eg: maybe we should also check what happens with xray-tail-call-sled.ll for the new default? and how about fast-isel-call.ll, toc-float.ll etc?

Generally the way I decided on how to update the test depended on a couple of things:

  1. If the test had this line: ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py it means that we had an auto generate test and if I felt that the test had nothing really to do with PIC or no-PIC I would just regenerate it. For example a number of tests just had addis ld differences.
  2. If the test was not auto generated but it still had nothing to do with PIC / no-PIC I would just add -relocation-model=pic. Changing these tests would not get us very much and it would be a significant amount of work.
  3. For tests that had something to do with PIC / no-PIC I tried to make a judgement call as to what would be more useful to keep (the PIC test or the non-PIC test). If you feel that some of these tests I should have been the other way around let me know. I know you mentioned a couple in your comment and I'll take a look at those.

The reason why it may look a little random is because of all of the testCompare* tests. I don't know why but it seems that some of those tests were auto-generated and some were not but they all have nothing to do with PIC.

joerg added a comment.Oct 24 2018, 1:35 PM

To summarize the discussion on IRC, from my perspective this change is wrong. PPC as architecture is strongly geared towards PIC code. We used to create quite bad position dependent code and I generally don't see the advantage. I can understand wanting to default to PIE (i.e. position independent code with main binary assumption as far as interception is considered). In other words, the middle end generally tended to create better results for position independent codegen. The default behavior of GCC is ephemeral here, IMO.

PPC as architecture is strongly geared towards PIC code"

Really some truth, as I know, AIX 64bit mode only supports PIC.

@joerg
When you say that we produce quite bad position dependent code do you mean in terms of performance or in terms of correctness? At this point I have not seen any issues with the no-PIC code. If you have an example of this bad code please share the test case.
Also, changing the default option to no-PIC does not mean that everything we do will become no-PIC. For example, the way that the TOC is handled on PPC remains position independent. However, going from PIC to no-PIC does offer a number of opportunities (for example in in-lining).
With respect to PIE I'm not sure what the difference is between PIE and static relocation models in terms of code generation. Is there any reason why you would prefer PIE in this case? Do you have an example that shows it to be better than say no-PIC?

@jedilyn
You are correct that PIC is the only model on AIX. However, the object model (COFF) on AIX is different so the idea of PIC / no-PIC does not mean the same thing it does on Linux anyway. Also AIX is currently not supported for LLVM. If we decide to support it we can look at the options then. However, since AIX only uses one object model and there is no PIC/ no-PIC difference we will probably just end up ignoring the option anyway.

@jedilyn
You are correct that PIC is the only model on AIX. However, the object model (COFF) on AIX is different so the idea of PIC / no-PIC does not mean the same thing it does on Linux anyway. Also AIX is currently not supported for LLVM. If we decide to support it we can look at the options then. However, since AIX only uses one object model and there is no PIC/ no-PIC difference we will probably just end up ignoring the option anyway.

Thanks for clarification Stefan, it's a good idea just to ignore the PIC option on aix when it gets supported in future.

sfertile accepted this revision.Nov 13 2018, 1:10 PM

I definitely agree we need to fix the PIC by default behavior, PIC implies we need to produce code suitable for a shared object and that has a number of unneeded limitations. I tried to figure out what we are doing differently between pie and static relocation models but don't see much difference.
For the most part we happen to do the same thing, for example the default isOffsetFoldingLegal depends on position-dependence but the PPCTargetLowering overrides it to always return false, we always produce relative jump-tables for ppc64 regardless of the relocation mode, we always use TOC-pointer relative addressing for module local symbols regardles of relocation model . The one difference I did find is TargetLoweringObjectFile::getKindForGlobal although that line looks wrong to me. Static binaries can still link against shared objects. Globals/functions defined in those will not have their addresses resolved at link time, unless maybe this is only a problem on ppc since we don't use copy relocs? (I believe the original PIC by default change was motivated by this but without a clear example I'm not sure why ...). I don't like using the fact there is (or was) bad codegen for the static relocation model as reasoning for picking pie as the default, that bad codegen comes back if the user adds -fno-pie as an option.

I think using static relocation model is the right approach here. I don't think we should deviate from the same default xlc and gcc uses without strong benefit to motivate the difference, and an understanding that the option difference will not impact compatibility.

LGTM

This revision is now accepted and ready to land.Nov 13 2018, 1:10 PM
This revision was automatically updated to reflect the committed changes.
stefanp reopened this revision.Nov 16 2018, 12:44 PM
This revision is now accepted and ready to land.Nov 16 2018, 12:44 PM
stefanp updated this revision to Diff 175299.Nov 26 2018, 11:08 AM

Patch is now only for Little Endian.
Big Endian will remain PIC by default as it was before. This is due to a bug found on BE where -fsanitize=leak is not detecting leaks correctly. As Little Endian is a higher priority we will add that code first and then look at the BE issue at a later date.

This revision was automatically updated to reflect the committed changes.