[AIX][XCOFF] Supporting the ReadOnlyWithRel SectionKnd

[DiggerLin]

Summary

in this patch we put the global variable in a Csect which's SectionKind is "ReadOnlyWithRel" into Data Section.

[hubert.reinterpretcast]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1873–1876

Please add a TODO for putting this under option control. The user may want to have read-only data with relocations placed into a read-only section by the compiler. There are linker options, such as -Wl,-bforceimprw (which would make sense to use alongside -fdata-sections), that support such cases.

[DiggerLin]

added a comment

[Xiangling_L]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1874

s/maybe/may ?

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1639

Considering this if clause is going longer when we support more GVKind, does that make sense to make the code cleaner by having a helper function like isValidGVKind?

llvm/test/CodeGen/PowerPC/aix-readonly-relocation.ll

1 ↗

(On Diff #237344)

I kinda feel the testcase name is a little misleading, is that better to call it aix-readonly-with-relocation.ll?

[DiggerLin]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1874

thanks,fixed

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1639

I do not think we need to add helper function here, I changed to simple condition. thank for let me to consider a simple one.

llvm/test/CodeGen/PowerPC/aix-readonly-relocation.ll

1 ↗

(On Diff #237344)

changed

[hubert.reinterpretcast]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1874

I'm still seeing "maybe". When saying something is fixed, please be clear if the fixed version is not being posted "immediately".

[hubert.reinterpretcast]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1876

Is seems SectionKind::isGlobalWriteableData should be updated in the case where we generate ReadOnlyWithRel as read-only unless if we find that the SectionKind is what we should change in that case.

[DiggerLin]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1876

1. I am not sure for OS, whether they also have the option for put the ReadOnlyWithRel in the readonly section or not ? this is common part for all the OS .
2. even if when we decide to change the SectionKind::isGlobalWriteableData, we will create a new patch for it.

[DiggerLin]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1876

sorry , it should be I am not sure for other OS(not xcoff)

[DiggerLin]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1873–1876

and if we put the ReadOnlyWithRel as read-only section. I think the function bool isReadOnly() const {

  return K == ReadOnly || isMergeableCString() ||
         isMergeableConst();
} also need to be modified.

We can put the change of SectionKind::isGlobalWriteableData() and isReadOnly() in the same new patch later.

[hubert.reinterpretcast]

LGTM with just the fix to replace the "maybe"s. What we should do for the later TODO can be determined later.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

return getContext().getXCOFFSection(
         Name, XCOFF::XMC_RO, XCOFF::XTY_SD,
         TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GO),
         Kind, /* BeginSymbolName */ nullptr);
   }
 
   if (Kind.isText())
     return TextSection;
 
-  if (Kind.isData())
+  if (Kind.isData() || Kind.isReadOnlyWithRel())
+    // TODO: We may put this under option control, because user may want to
+    // have read-only data with relocations placed into a read-only section by
+    // the compiler.
     return DataSection;
 
   // 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.isBSS())
     return DataSection;
 

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

if (!GV->hasInitializer())
     return;
 
   // Create the symbol, set its storage class.
   MCSymbolXCOFF *GVSym = cast<MCSymbolXCOFF>(getSymbol(GV));
   GVSym->setStorageClass(
       TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GV));
 
   SectionKind GVKind = getObjFileLowering().getKindForGlobal(GV, TM);
-  if ((!GVKind.isCommon() && !GVKind.isBSS() && !GVKind.isData() &&
-       !GVKind.isReadOnly()) ||
+  if ((!GVKind.isGlobalWriteableData() && !GVKind.isReadOnly()) ||
       GVKind.isMergeable2ByteCString() || GVKind.isMergeable4ByteCString())
     report_fatal_error("Encountered a global variable kind that is "
                        "not supported yet.");
 
   // Create the containing csect and switch to it.
   MCSectionXCOFF *Csect = cast<MCSectionXCOFF>(
       getObjFileLowering().SectionForGlobal(GV, GVKind, TM));
   OutStreamer->SwitchSection(Csect);

llvm/test/CodeGen/PowerPC/aix-readonly-with-relocation.ll

+; RUN: llc -verify-machineinstrs -mcpu=pwr4 -mtriple powerpc-ibm-aix-xcoff --relocation-model=pic < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -mcpu=pwr4 -mtriple powerpc64-ibm-aix-xcoff --relocation-model=pic < %s | FileCheck --check-prefix=CHECK64 %s
+
+@a = common global i32 0
+@b = constant i32* @a
+
+;CHECK:         .comm   a[RW],4,2
+;CHECK-NEXT:    .csect .data[RW]
+;CHECK-NEXT:    .globl  b
+;CHECK-NEXT:    .align  2
+;CHECK-NEXT: b:
+;CHECK-NEXT:    .long   a
+
+;CHECK64:       .comm   a[RW],4,2
+;CHECK64-NEXT:  .csect .data[RW]
+;CHECK64-NEXT:  .globl  b
+;CHECK64-NEXT:  .align  3
+;CHECK64-NEXT: b:
+;CHECK64-NEXT:  .llong  a