This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][AIX] Make PIC the default relocation model for AIX
ClosedPublic

Authored by stevewan on Jan 9 2020, 1:36 PM.

Details

Summary

The llc tool currently defaults to Static relocation model and generates non-relocatable code for 32-bit Power.
This is not desirable on AIX where we always generate Position Independent Code (PIC). This patch makes PIC the default relocation model for AIX.

Event Timeline

stevewan created this revision.Jan 9 2020, 1:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2020, 1:36 PM
llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
237

Would it make sense to disable setting anything other than PIC for AIX as part of this work?

llvm/test/CodeGen/PowerPC/aix-pic-default.ll
1 ↗(On Diff #237187)

I think using an API unit test instead of a LIT test would be more direct.

stevewan updated this revision to Diff 237434.Jan 10 2020, 2:29 PM
stevewan marked 4 inline comments as done.

Address comments to:

  1. disable setting anything other than PIC for AIX,
  2. add an API unit test instead of LIT test.
stevewan added inline comments.Jan 10 2020, 2:30 PM
llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
237

True, AIX only supports PIC.

llvm/test/CodeGen/PowerPC/aix-pic-default.ll
1 ↗(On Diff #237187)

Sure, add it to "llvm/unittests/CodeGen/" I guess?

llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
226

I meant that the user-facing option processing code should generate an error message to prevent setting something other than PIC for AIX.

The code here should assert !RM.hasValue() || *RM == Reloc::PIC_ for AIX.

llvm/unittests/CodeGen/CMakeLists.txt
17 ↗(On Diff #237434)

Given the location of the code change, I think unittests/Target/PowerPC would be where the new test would go?

stevewan marked 3 inline comments as done.Jan 14 2020, 2:10 PM
stevewan added inline comments.
llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
226

As per personal discussion, the target could be unknown during option processing and therefore RM settings will need to be determined after that phase.

An assertion will be added to this getter function, and the user error will be reported in the llc code before the powerpc target machine is created and this getter function is called, because the PPCTargetMachine constructor doesn't have an existing mechanism to report user errors.

stevewan updated this revision to Diff 238098.Jan 14 2020, 2:20 PM
stevewan marked an inline comment as done.

Address comments to:

  1. add assertion and user error report,
  2. relocate unit test.

@stevewan, can you confirm if there are other tools/utilities that need similar treatment?

llvm/tools/llc/llc.cpp
399

I suggest moving this to immediately before the use.

464

s/will result in an user error/is considered a user error/;

llvm/unittests/Target/PowerPC/AIXRelocModelTest.cpp
28

powerpc--aix.

36

s/force/forced/;
s/regardeless/regardless/;

stevewan updated this revision to Diff 238265.Jan 15 2020, 8:08 AM

Fix typos and address readability concerns.

stevewan marked 4 inline comments as done.Jan 15 2020, 8:09 AM
stevewan edited the summary of this revision. (Show Details)
llvm/tools/llc/llc.cpp
469

A "lit" test that checks for this error message for would be appropriate. I would suggest checking that setting PIC explicitly works for both 32-bit and 64-bit AIX and setting other-than-PIC generates an error message for the same.

stevewan updated this revision to Diff 238403.Jan 15 2020, 6:42 PM

Add a lit test case.

stevewan marked 2 inline comments as done.Jan 15 2020, 6:45 PM
stevewan added inline comments.
llvm/tools/llc/llc.cpp
469

Thanks for pointing out, LIT case added.

stevewan marked an inline comment as done.Jan 15 2020, 6:48 PM
llvm/test/tools/llc/aix-pic-setting.ll
1

Replace 2>&1 with 2>&1 1>/dev/null on the RUN lines using CHECK-EMPTY.

7

I think we are actually expecting no output on stderr:

CHECK-EMPTY-NOT: {{.}}
stevewan updated this revision to Diff 238503.Jan 16 2020, 7:56 AM
stevewan marked 3 inline comments as done.

Address comments regarding the lit test case.

llvm/test/tools/llc/aix-pic-setting.ll
7

Thanks for noting. I tried to check for emptiness, but this look like the proper way of doing it. Also to avoid confusion, I'm omitting the CHECK-EMPTY prefix as it is already a predefined directive in FileCheck.

This revision is now accepted and ready to land.Jan 16 2020, 8:10 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jan 16 2020, 10:18 AM
thakis added inline comments.
llvm/unittests/Target/PowerPC/CMakeLists.txt
17

Most of these look unused. You shouldn't need Analysis CodeGen Core MC MIRParser, only Support Target PowerPC*

Also, this breaks tests on Windows: http://45.33.8.238/win/5946/step_11.txt

Thanks for noting, looking into it now.

In addition to the lit issue on Windows, it looks like the test is also missing a REQUIRES: powerpc-registered-target line: http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/12986/steps/ninja%20check%201/logs/FAIL%3A%20LLVM%3A%3Aaix-pic-setting.ll

llvm/test/tools/llc/aix-pic-setting.ll
3

The integrated shell is probably missing redirection support for stdin using 1> syntax. Plain > should work.

Also, this breaks tests on Windows: http://45.33.8.238/win/5946/step_11.txt

Thanks for noting, looking into it now.

Any luck? Bot's been broken for a while now. Maybe just remove the 1>/dev/null for now, or remove that test until things are figured out?

Also, this breaks tests on Windows: http://45.33.8.238/win/5946/step_11.txt

Thanks for noting, looking into it now.

Any luck? Bot's been broken for a while now. Maybe just remove the 1>/dev/null for now, or remove that test until things are figured out?

Added the REQUIRES directive which fixed part of the broken bots. The fix for the redirecting issue on Windows is also in, I'm monitoring the bots to validate.

stevewan marked 2 inline comments as done.Jan 16 2020, 1:19 PM
stevewan added inline comments.
llvm/unittests/Target/PowerPC/CMakeLists.txt
17

Addressed in a follow-on patch, rGfc4e43ad618b. Thanks!

stevewan marked an inline comment as done.Jan 16 2020, 1:21 PM
stevewan marked an inline comment as done.
stevewan added a project: Restricted Project.Jan 30 2020, 7:19 AM