[PATCH] D71551: [AIX][XCOFF]Implement mergeable const

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/MC/MCObjectFileInfo.h:176
 
-  // ELF specific sections.
+  // ELF and XCOFF pecific sections.
   MCSection *DataRelROSection = nullptr;
----------------
Typo: s/pecific/specific/;

This comment is misleading though. XCOFF does not define such sections, and the linker does not indicate it has support for the implied merging functionality.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1882
 
-  if (Kind.isReadOnly() && !Kind.isMergeableConst())
+  if (Kind.isMergeableConst4() && MergeableConst4Section)
+    return MergeableConst4Section;
----------------
This code is in an XCOFF-specific function. Is there a case where the pointer remains null? If there is, I suggest to test that case. If there isn't, I suggest to assert that the pointer is non-null instead of doing a null check.

This comment also applies to the next handful of lines.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1891
+
+  if (Kind.isReadOnly())
     return ReadOnlySection;
----------------
If we believe that there should be special mergeable const sections, then we should assert here that we have handled all mergeable const section variants (in case additional ones are added in the future).


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll:1
+;This file tests the codegen of mergeable const in AIX assembly
+; RUN: llc -verify-machineinstrs -mcpu=pwr9 -mtriple powerpc-ibm-aix-xcoff < %s | FileCheck %s
----------------
Period at the end of the sentence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71551