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

[stevewan]

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.

[hubert.reinterpretcast]

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]

Address comments to:

1. disable setting anything other than PIC for AIX,
2. add an API unit test instead of LIT test.

[stevewan]

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?

[hubert.reinterpretcast]

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]

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]

Address comments to:

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

[hubert.reinterpretcast]

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

Fix typos and address readability concerns.

[hubert.reinterpretcast]

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]

Add a lit test case.

[stevewan]

llvm/tools/llc/llc.cpp

469

Thanks for pointing out, LIT case added.

[hubert.reinterpretcast]

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

2

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

8

I think we are actually expecting no output on stderr:

CHECK-EMPTY-NOT: {{.}}

[stevewan]

Address comments regarding the lit test case.

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

8

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.

[hubert.reinterpretcast]

LGTM.

[thakis]

llvm/unittests/Target/PowerPC/CMakeLists.txt

16

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

[thakis]

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

[stevewan]

In D72479#1824564, @thakis wrote:

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

Thanks for noting, looking into it now.

[thakis]

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

[hubert.reinterpretcast]

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

2

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

[thakis]

In D72479#1824577, @stevewan wrote:

In D72479#1824564, @thakis wrote:

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?

[stevewan]

In D72479#1824697, @thakis wrote:

In D72479#1824577, @stevewan wrote:

In D72479#1824564, @thakis wrote:

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.

[thakis]

Thanks, Windows cycled green: http://45.33.8.238/win/5959/summary.html

[stevewan]

llvm/unittests/Target/PowerPC/CMakeLists.txt

16

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

llvm/lib/Target/PowerPC/PPCTargetMachine.cpp

case Triple::ppc64:
     return PPCTargetMachine::PPC_ABI_ELFv1;
   default:
     return PPCTargetMachine::PPC_ABI_UNKNOWN;
   }
 }
 
 static Reloc::Model getEffectiveRelocModel(const Triple &TT,
                                            Optional<Reloc::Model> RM) {
+  assert((!TT.isOSAIX() || !RM.hasValue() || *RM == Reloc::PIC_) &&
+         "Invalid relocation model for AIX.");
+
   if (RM.hasValue())
     return *RM;
 
   // Darwin defaults to dynamic-no-pic.
   if (TT.isOSDarwin())
     return Reloc::DynamicNoPIC;
 
-  // Big Endian PPC is PIC by default.
-  if (TT.getArch() == Triple::ppc64)
+  // Big Endian PPC and AIX default to PIC.
+  if (TT.getArch() == Triple::ppc64 || TT.isOSAIX())
     return Reloc::PIC_;
 
   // Rest are static by default.
   return Reloc::Static;
 }
 
 static CodeModel::Model getEffectivePPCCodeModel(const Triple &TT,
                                                  Optional<CodeModel::Model> CM,

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

+; RUN: llc -mtriple=powerpc-ibm-aix < %s 2>&1 1>/dev/null | FileCheck --allow-empty %s
+; RUN: llc -mtriple=powerpc-ibm-aix --relocation-model=pic < %s 2>&1 1>/dev/null | FileCheck --allow-empty %s
+; RUN: llc -mtriple=powerpc64-ibm-aix --relocation-model=pic < %s 2>&1 1>/dev/null | FileCheck --allow-empty %s
+; RUN: not llc -mtriple=powerpc-ibm-aix --relocation-model=static < %s 2>&1 | FileCheck --check-prefix=CHECK-NON-PIC %s
+; RUN: not llc -mtriple=powerpc64-ibm-aix --relocation-model=ropi-rwpi < %s 2>&1 | FileCheck --check-prefix=CHECK-NON-PIC %s
+
+; CHECK-NOT: {{.}}
+; CHECK-NON-PIC: invalid relocation model, AIX only supports PIC.

llvm/tools/llc/llc.cpp

 
 static int compileModule(char **argv, LLVMContext &Context) {
   // Load the module to be compiled...
   SMDiagnostic Err;
   std::unique_ptr<Module> M;
   std::unique_ptr<MIRParser> MIR;
   Triple TheTriple;
   std::string CPUStr = getCPUStr(), FeaturesStr = getFeaturesStr();
 
   // Set attributes on functions as loaded from MIR from command line arguments.
   auto setMIRFunctionAttributes = [&CPUStr, &FeaturesStr](Function &F) {
     setFunctionAttributes(CPUStr, FeaturesStr, F);
   };
 
   bool SkipModule = MCPU == "help" ||
                     (!MAttrs.empty() && MAttrs.front() == "help");
 
static int compileModule(char **argv, LLVMContext &Context) {
   Options.DisableIntegratedAS = NoIntegratedAssembler;
   Options.MCOptions.ShowMCEncoding = ShowMCEncoding;
   Options.MCOptions.MCUseDwarfDirectory = EnableDwarfDirectory;
   Options.MCOptions.AsmVerbose = AsmVerbose;
   Options.MCOptions.PreserveAsmComments = PreserveComments;
   Options.MCOptions.IASSearchPaths = IncludeDirs;
   Options.MCOptions.SplitDwarfFile = SplitDwarfFile;
 
+  // On AIX, setting the relocation model to anything other than PIC is considered
+  // a user error.
+  Optional<Reloc::Model> RM = getRelocModel();
+  if (TheTriple.isOSAIX() && RM.hasValue() && *RM != Reloc::PIC_) {
+    WithColor::error(errs(), argv[0])
+        << "invalid relocation model, AIX only supports PIC.\n";
+    return 1;
+  }
+
   std::unique_ptr<TargetMachine> Target(TheTarget->createTargetMachine(
-      TheTriple.getTriple(), CPUStr, FeaturesStr, Options, getRelocModel(),
+      TheTriple.getTriple(), CPUStr, FeaturesStr, Options, RM,
       getCodeModel(), OLvl));
 
   assert(Target && "Could not allocate target machine!");
 
   // If we don't have a module then just exit now. We do this down
   // here since the CPU/Feature help is underneath the target machine
   // creation.
   if (SkipModule)

llvm/unittests/Target/PowerPC/AIXRelocModelTest.cpp

+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "llvm/Target/TargetMachine.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+class AIXRelocModelTest : public ::testing::Test {
+protected:
+  static void SetUpTestCase() {
+    LLVMInitializePowerPCTargetInfo();
+    LLVMInitializePowerPCTarget();
+    LLVMInitializePowerPCTargetMC();
+  }
+};
+
+TEST_F(AIXRelocModelTest, DefalutToPIC) {
+  Triple TheTriple(/*ArchStr*/ "powerpc", /*VendorStr*/ "", /*OSStr*/ "aix");
+  std::string Error;
+  const Target *TheTarget = TargetRegistry::lookupTarget("", TheTriple, Error);
+  ASSERT_TRUE(TheTarget) << Error;
+
+  TargetOptions Options;
+  // Create a TargetMachine for powerpc--aix target, and deliberately leave its
+  // relocation model unset.
+  std::unique_ptr<TargetMachine> Target(TheTarget->createTargetMachine(
+      /*TT*/ TheTriple.getTriple(), /*CPU*/ "", /*Features*/ "",
+      /*Options*/ Options, /*RM*/ None, /*CM*/ None,
+      /*OL*/ CodeGenOpt::Default));
+  ASSERT_TRUE(Target) << "Could not allocate target machine!";
+
+  // The relocation model on AIX should be forced to PIC regardless.
+  EXPECT_TRUE(Target->getRelocationModel() == Reloc::PIC_);
+}
+
+} // end of anonymous namespace

llvm/unittests/Target/PowerPC/CMakeLists.txt

+include_directories(
+  ${CMAKE_SOURCE_DIR}/lib/Target/PowerPC
+  ${CMAKE_BINARY_DIR}/lib/Target/PowerPC
+  )
+
+set(LLVM_LINK_COMPONENTS
+  Analysis
+  CodeGen
+  Core
+  MC
+  MIRParser
+  Support
+  Target
+  PowerPCCodeGen
+  PowerPCDesc
+  PowerPCInfo
+  )
+
+add_llvm_unittest(PowerPCTests
+  AIXRelocModelTest.cpp
+  )