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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1870
 
+  if (Kind.isMergeableConst()) {
+    return getContext().getXCOFFSection(".rodata.cst",
----------------
As @jasonliu mentioned, there is not much precedent for this in the context of AIX. What is the rationale for not using the normal read-only data section?


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1888
 
   if (Kind.isReadOnly() && !Kind.isMergeableConst())
     return ReadOnlySection;
----------------
`!Kind.isMergeableConst()` is always true here now. There should be no need to check that here.


================
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
----------------
This is also testing the object file generation?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll:2
+;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
+; RUN: llc -verify-machineinstrs -mcpu=pwr9 -mtriple powerpc-ibm-aix-xcoff -filetype=obj -o %t.o < %s
----------------
Should the assembly path be tested for 64-bit as well?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll:3
+; RUN: llc -verify-machineinstrs -mcpu=pwr9 -mtriple powerpc-ibm-aix-xcoff < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -mcpu=pwr9 -mtriple powerpc-ibm-aix-xcoff -filetype=obj -o %t.o < %s
+; RUN: llvm-objdump -D %t.o | FileCheck --check-prefix=CHECKOBJ %s
----------------
Why `pwr9`?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll:5
+; RUN: llvm-objdump -D %t.o | FileCheck --check-prefix=CHECKOBJ %s
+; RUN:  llvm-readobj -syms %t.o | FileCheck --check-prefix=CHECKSYM %s
+
----------------
There's two spaces after the colon here (but one space after the colon on the previous lines).


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll:12
+
+ at __const.main.cnst32 = private unnamed_addr constant %struct.Merge_cnst32 { i64 4611686018427387954, i32 0, i64 0, i32 0 }, align 8
+ at __const.main.cnst16 = private unnamed_addr constant %struct.Merge_cnst16 { i64 4611686018427387926, i32 0 }, align 8
----------------
The highest-alignment-first ordering here is less interesting than the reverse order in terms of how padding is generated.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll:94
+;CHECKSYM-NEXT:       Index: [[#Index+3]]
+;CHECKSYM-NEXT:       ContainingCsectSymbolIndex: 4
+;CHECKSYM-NEXT:       ParameterHashIndex: 0x0
----------------
Shouldn't this be based on `Index`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71551