[PATCH] D72461: [AIX][XCOFF] Supporting the ReadOnlyWithRel SectionKnd

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1874
+  if (Kind.isData() || Kind.isReadOnlyWithRel())
+    // TODO: We maybe put this under option control, because user may want to
+    // have read-only data with relocations placed into a read-only section by
----------------
Xiangling_L wrote:
> s/maybe/may ?
thanks,fixed


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1802
   if ((!GVKind.isCommon() && !GVKind.isBSS() && !GVKind.isData() &&
-       !GVKind.isReadOnly()) ||
+       !GVKind.isReadOnly() && !GVKind.isReadOnlyWithRel()) ||
       GVKind.isMergeable2ByteCString() || GVKind.isMergeable4ByteCString())
----------------
Xiangling_L wrote:
> 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`? 
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.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-readonly-relocation.ll:1
+; 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
----------------
Xiangling_L wrote:
> I kinda feel the testcase name is a little misleading, is that better to call it `aix-readonly-with-relocation.ll`?
changed


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72461/new/

https://reviews.llvm.org/D72461