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

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L 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
----------------
s/maybe/may ?


================
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())
----------------
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`? 


================
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
----------------
I kinda feel the testcase name is a little misleading, is that better to call it `aix-readonly-with-relocation.ll`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72461