Page MenuHomePhabricator

[AIX][XCOFF] Supporting the ReadOnlyWithRel SectionKnd
ClosedPublic

Authored by DiggerLin on Thu, Jan 9, 8:44 AM.

Details

Summary

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

Diff Detail

Event Timeline

DiggerLin created this revision.Thu, Jan 9, 8:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jan 9, 8:44 AM
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 updated this revision to Diff 237344.Fri, Jan 10, 8:44 AM

added a comment

Xiangling_L added inline comments.
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 updated this revision to Diff 237761.Mon, Jan 13, 1:07 PM
DiggerLin marked 5 inline comments as done.
DiggerLin added inline comments.
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

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".

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 marked 2 inline comments as done.Tue, Jan 14, 7:11 AM
DiggerLin added inline comments.
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 marked 2 inline comments as done.Tue, Jan 14, 7:12 AM
DiggerLin added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1876

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

DiggerLin marked an inline comment as done.Tue, Jan 14, 7:26 AM
DiggerLin added inline comments.
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.

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

This revision is now accepted and ready to land.Tue, Jan 14, 8:01 AM
This revision was automatically updated to reflect the committed changes.