[AIX][CodeGen] Storage Locations for Constant Pointers

[qiongsiwu1]

Summary

This patch adds an llc option -mroptr to specify storage locations for constant pointers on AIX.

When the -mroptr option is specified, constant pointers, virtual function tables, and virtual type tables are placed in read-only storage. Otherwise, by default, pointers, virtual function tables, and virtual type tables are placed are placed in read/write storage.

https://reviews.llvm.org/D144190 enables the -mroptr option for clang.

[qiongsiwu1]

Clang format update.

[myhsu]

just wondering if there is any reason you chose not to implement this as a subtarget feature?

[qiongsiwu1]

In D144189#4132940, @myhsu wrote:

just wondering if there is any reason you chose not to implement this as a subtarget feature?

Thanks for the comment! No we did not consider that approach. How would this be implemented as a subtarget feature? Could you show me an existing example of what we mean by a "subtarget feature"?

[myhsu]

In D144189#4134864, @qiongsiwu1 wrote:

In D144189#4132940, @myhsu wrote:

just wondering if there is any reason you chose not to implement this as a subtarget feature?

Thanks for the comment! No we did not consider that approach. How would this be implemented as a subtarget feature? Could you show me an existing example of what we mean by a "subtarget feature"?

I think -mpcrel / pcrelative-memops in PPC is a good example. First, you can find pcrelative-memops's subtarget feature definition in llvm/lib/Target/PowerPC/PPC.td, which can be accessed by PPCSubtargetInfo::HasPCRelativeMemops. Then, in Clang, you can find mpcrel 's definition in clang/include/clang/Driver/Options.td. Such flag is put under the m_ppc_Features_Group so it will eventually be marshaled and piped into PPCTargetInfo::setFeatureEnabled (and some other PPCTargetInfo methods) in which you can translate it into LLVM pcrelative-memops target feature.

[qiongsiwu1]

In D144189#4139426, @myhsu wrote:

In D144189#4134864, @qiongsiwu1 wrote:

In D144189#4132940, @myhsu wrote:

just wondering if there is any reason you chose not to implement this as a subtarget feature?

Thanks for the comment! No we did not consider that approach. How would this be implemented as a subtarget feature? Could you show me an existing example of what we mean by a "subtarget feature"?

I think -mpcrel / pcrelative-memops in PPC is a good example. First, you can find pcrelative-memops's subtarget feature definition in llvm/lib/Target/PowerPC/PPC.td, which can be accessed by PPCSubtargetInfo::HasPCRelativeMemops. Then, in Clang, you can find mpcrel 's definition in clang/include/clang/Driver/Options.td. Such flag is put under the m_ppc_Features_Group so it will eventually be marshaled and piped into PPCTargetInfo::setFeatureEnabled (and some other PPCTargetInfo methods) in which you can translate it into LLVM pcrelative-memops target feature.

Ah I see! Thanks so much for the suggestion and clarification!

I took a closer look and discussed with other IBM colleagues and we think that we would like to keep the current implementation. The key reason is that this is not an PPC subtarget feature, rather a codegen feature for AIX/XCOFF. It has more to do with object file formats, linkers, and loaders. We are not offering this specific option on Linux.

Implementation-wise, as far as I can see, it is not easy to route the subtarget information to TargetLoweringObjectFileXCOFF, where we actually implement the logic for this option. A similar existing option is TargetOptions::IgnoreXCOFFVisibility.

That said, I may still be missing other aspects of the subtarget suggestion that could overcome the points above. Please let me know and I will look into them! Thanks again!

[qiongsiwu1]

Gentle ping for review. Thanks!

[stephenpeckham]

I don't see any issues with this code.

[qiongsiwu1]

Revise llc error checking. Adding a check for AIX.

[hubert.reinterpretcast]

llvm/docs/ReleaseNotes.rst

105–110

The requirement for use with data-sections is a design intent and not merely an implementation artifact. This may be a reasonable place to elaborate on that. Should also add additional comments in the code.

[qiongsiwu1]

Updating the release note and adding comments on the data-sections check.

[hubert.reinterpretcast]

llvm/tools/llc/llc.cpp

502

Other calls to reportError do not appear to use periods at the end of the message.

506–509

Suggestion: Adjusted/expanded code comment

511

Same comment as above regarding ending full-stop.

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-xcoff-roptr.ll

6

Please add --symbol-description (and observe that the storage mapping class is displayed as well).

23

Would OBJ-DAG allow the test to be written with the check lines in the order of the IR definitions?

30–32

Updating with the --symbol-description output would really help here.

39–42

dd is not the interesting at the IR level.
Also, all of the C++ implementation artifacts below are unchecked anyway.

llvm/test/CodeGen/PowerPC/aix64-xcoff-roptr.ll

2

Same comments as for the 32-bit case.

[qiongsiwu1]

Thanks for the comments @hubert.reinterpretcast !! The patch is updated.

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-xcoff-roptr.ll

23

I suggest replacing all of the address and symbol table index fields with regular expressions.

29–30

The TOC entries are not interesting for our purposes here.

38

I think the rest of the file (including the definition of dd) can be removed.

llvm/test/CodeGen/PowerPC/aix64-xcoff-roptr.ll

2

Same new comments as for the 32-bit case.

[qiongsiwu1]

Ping for review. Thanks!

[qiongsiwu1]

Updating test cases to address review comments. A test for int *const x = (int *)0x1; is also added.

[nemanjai]

I think the implementation is fine, but just want to bring something else up for consideration wrt. to this option. As far as I can tell, nothing about this option is communicated in the IR. So if I split up IR generation (with clang -emit-llvm) and code generation (with opt/llc), the pointers will be placed into read-only sections only if -mroptr is specified to llc. While this isn't a common way users use the compiler, it may not be a concern. However, what are the implications of an LTO build. If we specify -mroptr on the compile steps but not on the link step, we won't get the behaviour. Presumably this is acceptable (i.e. the link will probably also fail if we forget to specify -flto on the link step) but it might be useful to document that somewhere.

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-xcoff-roptr.ll

32

This patch is not expected to change the behaviour for x in any way. Should the x case really be included? If it is, should the test for it not be for it being RO even with -mno-roptr?

[qiongsiwu1]

llvm/test/CodeGen/PowerPC/aix-xcoff-roptr.ll

32

Thanks for the comment! This test was added because I thought we may expect the default behaviour of such pointers (that they are going into the RO section) to stay. If there is a change in such default behaviour, this test case can catch it. Maybe there are other tests covering this exact case but I am not sure about that.

It sounds like that we do not need to have a test for this behaviour since it is not changed by this patch. I am removing this test.

[qiongsiwu1]

Address review comments - removing unnecessary tests.

[hubert.reinterpretcast]

This LGTM; thanks!

[hubert.reinterpretcast]

llvm/include/llvm/Target/TargetOptions.h

364

Can we rename this to XCOFFReadOnlyPointers?

[hubert.reinterpretcast]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

2436

The option seems out of the control of the back-end to me (it is "user input"). I believe report_fatal_error may be more appropriate.

[hubert.reinterpretcast]

llvm/tools/llc/llc.cpp

513

See https://reviews.llvm.org/D144190#inline-1406700 for why having this diagnostic in llc may not be the best.

[qiongsiwu1]

Rename the ReadOnlyPointers option in the source to XCOFFReadOnlyPointers.

[hubert.reinterpretcast]

llvm/tools/llc/llc.cpp

513

Since the back end now errors out reliably (report_fatal_error), the comment linked above for the other patch implies that we can remove this llc diagnostic in this patch. Then we can land this patch first.

[qiongsiwu1]

llvm/tools/llc/llc.cpp

513

Since the back end now errors out reliably (report_fatal_error), the comment linked above for the other patch implies that we can remove this llc diagnostic in this patch. Then we can land this patch first.

It seems that the report_fatal_error mechanism works more like an assert. llc crashes when encountering a report_fatal_error function. It is probably not ideal that we crash when -data-sections=false is passed through the command line. Should we keep this llc check? I am inclined to do so because I don't think we should allow llc to crash. That said, if the crashing behaviour is acceptable, I will remove the -data-sections=false test case, and remove this check. Alternatively, there may be other error reporting mechanisms in the backend that I am not aware of that can generate a valid error message without crashing. If there are, I will adopt them so we can report the error in the backend, and not crash.

[hubert.reinterpretcast]

Patch LGTM. Thanks for looking into the concerns.

llvm/tools/llc/llc.cpp

513

Since we have the diagnostic in llc (and for clang -cc1 in the other patch) already, and it is the most user-friendly behaviour, I think we are good to close on this.

Just noting that the report_fatal_error behaviour may be acceptable (in case we end up working on something similar in the future).

[qiongsiwu1]

Update comments and release notes.

[qiongsiwu1]

Fix clang-format.

[qiongsiwu1]

@arsenm @MaskRay and @serge-sans-paille, I would like to draw your attention to this patch. We would like to get your advice on the following issues:

1. We would like to do some error check in llc to guard against invalid option combinations. Doing the checks in llc's main feels out of place. Doing it in InitTargetOptionsFromCodeGenFlags() https://github.com/llvm/llvm-project/blob/4c82050c56926d840e4ccf253ad10e6ae3ee6cc7/llvm/lib/CodeGen/CommandFlags.cpp#L517 probably makes more sense, but we do not perform any error checks there at the moment. What is a good place to perform such error checks? This is related to https://reviews.llvm.org/D144190#4180329, where we are looking for advice on how to check codegen options when doing a pure linking job. What is the expected behaviour if an option that is not technically generic (such as this -mroptr option that only makes sense for XCOFF on AIX), but not architecture specific (in theory we may have XCOFF running on other platforms) either?
2. We are touching some target-independent files to implement this option. Earlier @myhsu pointed out that this might be a subtarget feature https://reviews.llvm.org/D144189#4132940 and we argued that it is not https://reviews.llvm.org/D144189#4144677 because this is an AIX/XCOFF issue. We would like to notify you all and make sure you are ok with this as well.

Thanks so much!!

FYI @nemanjai @hubert.reinterpretcast

[qiongsiwu1]

Ping for review/comment! Thanks so much!

[qiongsiwu1]

Ping for review/comment! Thanks so much!

[qiongsiwu1]

@arsenm @MaskRay and @serge-sans-paille, if there are no further feedback/comments, I will land this patch tomorrow (March 23, 2023). Thanks so much for your help!

[MaskRay]

llvm/docs/ReleaseNotes.rst

109

s/available/only supported/

llvm/test/CodeGen/PowerPC/aix64-xcoff-roptr.ll

17

;CHECK => ; CHECK

21

Replace {{[0-9]+}} with [[#]]

[qiongsiwu1]

Address code review. Thanks @MaskRay for the comments and approval!

[serge-sans-paille]

No comment, thanks for the patch!

llvm/docs/ReleaseNotes.rst

 Changes to the MIPS Backend
 ---------------------------
 
 * ...
 
 Changes to the PowerPC Backend
 ------------------------------
 
-* ...
+* A new option ``-mroptr`` is added to ``clang`` and ``llc``. When this option
+  is present, constant objects with relocatable address values are put into the
+  RO data section. This option should be used with the ``-fdata-sections``
+  option, and is not supported with ``-fno-data-sections``. The option is
+  available on AIX.
 
 Changes to the RISC-V Backend
 -----------------------------
 
 * Assembler support for version 1.0.1 of the Zcb extension was added.
 * Zca, Zcf, and Zcd extensions were upgraded to version 1.0.1.
 * vsetvli intrinsics no longer have side effects. They may now be combined,
   moved, deleted, etc. by optimizations.
 * Adds support for the vendor-defined XTHeadBa (address-generation) extension.

llvm/include/llvm/CodeGen/CommandFlags.h

 bool getXRayOmitFunctionIndex();
 
 bool getDebugStrictDwarf();
 
 unsigned getAlignLoops();
 
 bool getJMCInstrument();
 
+bool getXCOFFReadOnlyPointers();
+
 /// Create this object with static storage to register codegen-related command
 /// line options.
 struct RegisterCodeGenFlags {
   RegisterCodeGenFlags();
 };
 
 llvm::BasicBlockSection getBBSectionsMode(llvm::TargetOptions &Options);
 

llvm/include/llvm/Target/TargetOptions.h

namespace llvm {
   public:
     TargetOptions()
         : UnsafeFPMath(false), NoInfsFPMath(false), NoNaNsFPMath(false),
           NoTrappingFPMath(true), NoSignedZerosFPMath(false),
           ApproxFuncFPMath(false), EnableAIXExtendedAltivecABI(false),
           HonorSignDependentRoundingFPMathOption(false), NoZerosInBSS(false),
           GuaranteedTailCallOpt(false), StackSymbolOrdering(true),
           EnableFastISel(false), EnableGlobalISel(false), UseInitArray(false),
-          DisableIntegratedAS(false),
-          RelaxELFRelocations(true), FunctionSections(false),
-          DataSections(false), IgnoreXCOFFVisibility(false),
-          XCOFFTracebackTable(true), UniqueSectionNames(true),
-          UniqueBasicBlockSectionNames(false), TrapUnreachable(false),
-          NoTrapAfterNoreturn(false), TLSSize(0), EmulatedTLS(false),
-          ExplicitEmulatedTLS(false), EnableIPRA(false),
+          DisableIntegratedAS(false), RelaxELFRelocations(true),
+          FunctionSections(false), DataSections(false),
+          IgnoreXCOFFVisibility(false), XCOFFTracebackTable(true),
+          UniqueSectionNames(true), UniqueBasicBlockSectionNames(false),
+          TrapUnreachable(false), NoTrapAfterNoreturn(false), TLSSize(0),
+          EmulatedTLS(false), ExplicitEmulatedTLS(false), EnableIPRA(false),
           EmitStackSizeSection(false), EnableMachineOutliner(false),
           EnableMachineFunctionSplitter(false), SupportsDefaultOutlining(false),
           EmitAddrsig(false), EmitCallSiteInfo(false),
           SupportsDebugEntryValues(false), EnableDebugEntryValues(false),
           ValueTrackingVariableLocations(false), ForceDwarfFrameSection(false),
           XRayOmitFunctionIndex(false), DebugStrictDwarf(false),
           Hotpatch(false), PPCGenScalarMASSEntries(false), JMCInstrument(false),
-          EnableCFIFixup(false), MisExpect(false),
+          EnableCFIFixup(false), MisExpect(false), XCOFFReadOnlyPointers(false),
           FPDenormalMode(DenormalMode::IEEE, DenormalMode::IEEE) {}
 
     /// DisableFramePointerElim - This returns true if frame pointer elimination
     /// optimization should be disabled for the given machine function.
     bool DisableFramePointerElim(const MachineFunction &MF) const;
 
     /// If greater than 0, override the default value of
     /// MCAsmInfo::BinutilsVersion.
public:
 
     /// Enable the CFIFixup pass.
     unsigned EnableCFIFixup : 1;
 
     /// When set to true, enable MisExpect Diagnostics
     /// By default, it is set to false
     unsigned MisExpect : 1;
 
+    /// When set to true, const objects with relocatable address values are put
+    /// into the RO data section.
+    unsigned XCOFFReadOnlyPointers : 1;
+
     /// Name of the stack usage file (i.e., .su file) if user passes
     /// -fstack-usage. If empty, it can be implied that -fstack-usage is not
     /// passed on the command line.
     std::string StackUsageOutput;
 
     /// If greater than 0, override TargetLoweringBase::PrefLoopAlignment.
     unsigned LoopAlignment = 0;
 

llvm/lib/CodeGen/CommandFlags.cpp

 CGOPT(bool, EmitCallSiteInfo)
 CGOPT(bool, EnableMachineFunctionSplitter)
 CGOPT(bool, EnableDebugEntryValues)
 CGOPT(bool, ForceDwarfFrameSection)
 CGOPT(bool, XRayOmitFunctionIndex)
 CGOPT(bool, DebugStrictDwarf)
 CGOPT(unsigned, AlignLoops)
 CGOPT(bool, JMCInstrument)
+CGOPT(bool, XCOFFReadOnlyPointers)
 
 codegen::RegisterCodeGenFlags::RegisterCodeGenFlags() {
 #define CGBINDOPT(NAME)                                                        \
   do {                                                                         \
     NAME##View = std::addressof(NAME);                                         \
   } while (0)
 
   static cl::opt<std::string> MArch(
#define CGBINDOPT(NAME)                                                        \
   CGBINDOPT(AlignLoops);
 
   static cl::opt<bool> JMCInstrument(
       "enable-jmc-instrument",
       cl::desc("Instrument functions with a call to __CheckForDebuggerJustMyCode"),
       cl::init(false));
   CGBINDOPT(JMCInstrument);
 
+  static cl::opt<bool> XCOFFReadOnlyPointers(
+      "mroptr",
+      cl::desc("When set to true, const objects with relocatable address "
+               "values are put into the RO data section."),
+      cl::init(false));
+  CGBINDOPT(XCOFFReadOnlyPointers);
+
 #undef CGBINDOPT
 
   mc::RegisterMCTargetOptionsFlags();
 }
 
 llvm::BasicBlockSection
 codegen::getBBSectionsMode(llvm::TargetOptions &Options) {
   if (getBBSections() == "all")
codegen::InitTargetOptionsFromCodeGenFlags(const Triple &TheTriple) {
   Options.EmitAddrsig = getEnableAddrsig();
   Options.EmitCallSiteInfo = getEmitCallSiteInfo();
   Options.EnableDebugEntryValues = getEnableDebugEntryValues();
   Options.ForceDwarfFrameSection = getForceDwarfFrameSection();
   Options.XRayOmitFunctionIndex = getXRayOmitFunctionIndex();
   Options.DebugStrictDwarf = getDebugStrictDwarf();
   Options.LoopAlignment = getAlignLoops();
   Options.JMCInstrument = getJMCInstrument();
+  Options.XCOFFReadOnlyPointers = getXCOFFReadOnlyPointers();
 
   Options.MCOptions = mc::InitMCTargetOptionsFromFlags();
 
   Options.ThreadModel = getThreadModel();
   Options.EABIVersion = getEABIVersion();
   Options.DebuggerTuning = getDebuggerTuningOpt();
   Options.SwiftAsyncFramePointer = getSwiftAsyncFramePointer();
   return Options;

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

if (GVar->hasAttribute("toc-data"))
       return getContext().getXCOFFSection(
           SectionName, Kind,
           XCOFF::CsectProperties(/*MappingClass*/ XCOFF::XMC_TD, XCOFF::XTY_SD),
           /* MultiSymbolsAllowed*/ true);
 
   XCOFF::StorageMappingClass MappingClass;
   if (Kind.isText())
     MappingClass = XCOFF::XMC_PR;
-  else if (Kind.isData() || Kind.isReadOnlyWithRel() || Kind.isBSS())
+  else if (Kind.isData() || Kind.isBSS())
     MappingClass = XCOFF::XMC_RW;
+  else if (Kind.isReadOnlyWithRel())
+    MappingClass =
+        TM.Options.XCOFFReadOnlyPointers ? XCOFF::XMC_RO : XCOFF::XMC_RW;
   else if (Kind.isReadOnly())
     MappingClass = XCOFF::XMC_RO;
   else
     report_fatal_error("XCOFF other section types not yet implemented.");
 
   return getContext().getXCOFFSection(
       SectionName, Kind, XCOFF::CsectProperties(MappingClass, XCOFF::XTY_SD),
       /* MultiSymbolsAllowed*/ true);
MCSection *TargetLoweringObjectFileXCOFF::SelectSectionForGlobal(
   if (Kind.isText()) {
     if (TM.getFunctionSections()) {
       return cast<MCSymbolXCOFF>(getFunctionEntryPointSymbol(GO, TM))
           ->getRepresentedCsect();
     }
     return TextSection;
   }
 
-  // TODO: We may put Kind.isReadOnlyWithRel() under option control, because
-  // user may want to have read-only data with relocations placed into a
-  // read-only section by the compiler.
+  if (TM.Options.XCOFFReadOnlyPointers && Kind.isReadOnlyWithRel()) {
+    if (!TM.getDataSections())
+      report_fatal_error(
+          "ReadOnlyPointers is supported only if data sections is turned on");
+
+    SmallString<128> Name;
+    getNameWithPrefix(Name, GO, TM);
+    return getContext().getXCOFFSection(
+        Name, SectionKind::getReadOnly(),
+        XCOFF::CsectProperties(XCOFF::XMC_RO, XCOFF::XTY_SD));
+  }
+
   // For BSS kind, zero initialized data must be emitted to the .data section
   // because external linkage control sections that get mapped to the .bss
   // section will be linked as tentative defintions, which is only appropriate
   // for SectionKind::Common.
   if (Kind.isData() || Kind.isReadOnlyWithRel() || Kind.isBSS()) {
     if (TM.getDataSections()) {
       SmallString<128> Name;
       getNameWithPrefix(Name, GO, TM);

llvm/test/CodeGen/PowerPC/aix-xcoff-roptr.ll

+; RUN: llc -mtriple powerpc-ibm-aix-xcoff -mroptr < %s | FileCheck %s
+; RUN: llc -mtriple powerpc-ibm-aix-xcoff -mroptr -filetype=obj -o %t.o < %s
+; RUN: llvm-objdump -t --symbol-description %t.o | FileCheck %s --check-prefix=OBJ
+
+; RUN: not llc -mtriple powerpc-ibm-aix-xcoff -mroptr -data-sections=false \
+; RUN: < %s 2>&1 | FileCheck %s --check-prefix=DS_ERR
+
+; DS_ERR: -mroptr option must be used with -data-sections
+
+%union.U = type { %"struct.U::A" }
+%"struct.U::A" = type { ptr }
+
+@_ZL1p = internal constant i32 ptrtoint (ptr @_ZL1p to i32), align 4
+;CHECK:         .csect _ZL1p[RO],2
+;CHECK-NEXT:    .lglobl _ZL1p[RO]
+;CHECK-NEXT:    .align  2
+;CHECK-NEXT:    .vbyte  4, _ZL1p[RO]
+;OBJ-DAG: {{([[:xdigit:]]{8})}} l .text {{([[:xdigit:]]{8})}} (idx: {{[0-9]+}}) _ZL1p[RO]
+@q = thread_local constant ptr @_ZL1p, align 4
+;CHECK:         .csect q[TL],2
+;CHECK-NEXT:    .globl  q[TL]
+;CHECK-NEXT:    .align  2
+;CHECK-NEXT:    .vbyte  4, _ZL1p[RO]
+;OBJ-DAG: {{([[:xdigit:]]{8})}} g O .tdata {{([[:xdigit:]]{8})}} (idx: {{[0-9]+}}) q[TL]
+@u = local_unnamed_addr constant [1 x %union.U] [%union.U { %"struct.U::A" { ptr @_ZL1p } }], align 4
+;CHECK:         .csect u[RO],2
+;CHECK-NEXT:    .globl  u[RO]
+;CHECK-NEXT:    .align  2
+;CHECK-NEXT:    .vbyte  4, _ZL1p[RO]
+;OBJ-DAG: {{([[:xdigit:]]{8})}} g .text {{([[:xdigit:]]{8})}} (idx: {{[0-9]+}}) u[RO]

llvm/test/CodeGen/PowerPC/aix64-xcoff-roptr.ll

+; RUN: llc -mtriple powerpc64-ibm-aix-xcoff -mroptr < %s | FileCheck %s
+; RUN: llc -mtriple powerpc64-ibm-aix-xcoff -mroptr -filetype=obj -o %t.o < %s
+; RUN: llvm-objdump -t --symbol-description %t.o | FileCheck %s --check-prefix=OBJ
+
+; RUN: not llc -mtriple powerpc64-ibm-aix-xcoff -mroptr -data-sections=false \
+; RUN: < %s 2>&1 | FileCheck %s --check-prefix=DS_ERR
+; RUN: not llc -mtriple powerpc64le-unknown-linux-gnu -mroptr \
+; RUN: < %s 2>&1 | FileCheck %s --check-prefix=OS_ERR
+
+; DS_ERR: -mroptr option must be used with -data-sections
+; OS_ERR: -mroptr option is only supported on AIX
+
+%union.U = type { %"struct.U::A" }
+%"struct.U::A" = type { ptr }
+
+@_ZL1p = internal constant i64 ptrtoint (ptr @_ZL1p to i64), align 8
+;CHECK:         .csect _ZL1p[RO],3
+;CHECK-NEXT:    .lglobl _ZL1p[RO]
+;CHECK-NEXT:    .align  3
+;CHECK-NEXT:    .vbyte  8, _ZL1p[RO]
+;OBJ-DAG: {{([[:xdigit:]]{16})}} l .text {{([[:xdigit:]]{16})}} (idx: {{[0-9]+}}) _ZL1p[RO]
+@q = thread_local constant ptr @_ZL1p, align 8
+;CHECK:         .csect q[TL],3
+;CHECK-NEXT:    .globl  q[TL]
+;CHECK-NEXT:    .align  3
+;CHECK-NEXT:    .vbyte  8, _ZL1p[RO]
+;OBJ-DAG: {{([[:xdigit:]]{16})}} g O .tdata {{([[:xdigit:]]{16})}} (idx: {{[0-9]+}}) q[TL]
+@u = local_unnamed_addr constant [1 x %union.U] [%union.U { %"struct.U::A" { ptr @_ZL1p } }], align 8
+;CHECK:         .csect u[RO],3
+;CHECK-NEXT:    .globl  u[RO]
+;CHECK-NEXT:    .align  3
+;CHECK-NEXT:    .vbyte  8, _ZL1p[RO]
+;OBJ-DAG: {{([[:xdigit:]]{16})}} g .text {{([[:xdigit:]]{16})}} (idx: {{[0-9]+}}) u[RO]

llvm/tools/llc/llc.cpp

if (V.consumeInteger(10, Num) || Num == 0 ||
       WithColor::error(errs(), argv[0])
           << "invalid -binutils-version, accepting 'none' or major.minor\n";
       return 1;
     }
   }
   TargetOptions Options;
   auto InitializeOptions = [&](const Triple &TheTriple) {
     Options = codegen::InitTargetOptionsFromCodeGenFlags(TheTriple);
+
+    if (Options.XCOFFReadOnlyPointers) {
+      if (!TheTriple.isOSAIX())
+        reportError("-mroptr option is only supported on AIX", InputFilename);
+
+      // Since the storage mapping class is specified per csect,
+      // without using data sections, it is less effective to use read-only
+      // pointers. Using read-only pointers may cause other RO variables in the
+      // same csect to become RW when the linker acts upon `-bforceimprw`;
+      // therefore, we require that separate data sections are used in the
+      // presence of ReadOnlyPointers. We respect the setting of data-sections
+      // since we have not found reasons to do otherwise that overcome the user
+      // surprise of not respecting the setting.
+      if (!Options.DataSections)
+        reportError("-mroptr option must be used with -data-sections",
+                    InputFilename);
+    }
+
     Options.BinutilsVersion =
         TargetMachine::parseBinutilsVersion(BinutilsVersion);
     Options.DisableIntegratedAS = NoIntegratedAssembler;
     Options.MCOptions.ShowMCEncoding = ShowMCEncoding;
     Options.MCOptions.AsmVerbose = AsmVerbose;
     Options.MCOptions.PreserveAsmComments = PreserveComments;
     Options.MCOptions.IASSearchPaths = IncludeDirs;
     Options.MCOptions.SplitDwarfFile = SplitDwarfFile;