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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 9 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1870
 
+  if (Kind.isMergeableConst()) {
+    return getContext().getXCOFFSection(".rodata.cst",
----------------
hubert.reinterpretcast wrote:
> 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?
I implemented the Mergeable const as gcc did.  
1 .gcc use a separate csect for all mergeable const. gcc generate is as 
  .csect _mergeableconst.ro_[RO],4
        .align 3
cnst32:
        .long   1073741824
        .long   50
        .space 24
        .globl cnst16
        .align 3
cnst16:
        .long   1073741824
        .long   22
        .space 8
        .globl cnst8
        .align 2
cnst8:
        .long   1073741832
        .space 4
        .globl cnst4
        .align 1
cnst4:
        .short  16392
        .space 2
        .csect .text[PR]
_section_.text:
        .csect .data[RW],4
        .long _section_.text
2. xlc use separate csect const32, const16,const8, const4 for mergeable const32 ,  mergeable const16, mergeable const8. mergeable const4.


       .csect  cnst32{RO}, 3
        .long   0x40000000              # "@\0\0\0"
        .long   0x00000032              # "\0\0\0002"
        .long   0x00000000              # "\0\0\0\0"
        .long   0x00000000              # "\0\0\0\0"
        .long   0x00000000              # "\0\0\0\0"
        .long   0x00000000              # "\0\0\0\0"
        .long   0x00000000              # "\0\0\0\0"
        .long   0x00000000              # "\0\0\0\0"

        .csect  cnst16{RO}, 3
        .long   0x40000000              # "@\0\0\0"
        .long   0x00000016              # "\0\0\0\026"
        .long   0x00000000              # "\0\0\0\0"
        .long   0x00000000              # "\0\0\0\0"

        .csect  cnst8{RO}, 3
        .long   0x40000008              # "@\0\0\b"
        .long   0x00000000              # "\0\0\0\0"

        .csect  cnst4{RO}
        .long   0x40080000              # "@\b\0\0"



================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1888
 
   if (Kind.isReadOnly() && !Kind.isMergeableConst())
     return ReadOnlySection;
----------------
hubert.reinterpretcast wrote:
> `!Kind.isMergeableConst()` is always true here now. There should be no need to check that here.
yes, we already deal with Kind.isMergeableConst() before the condition. I will delete it.


================
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
----------------
hubert.reinterpretcast wrote:
> Should the assembly path be tested for 64-bit as well?
added


================
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
----------------
hubert.reinterpretcast wrote:
> Why `pwr9`?
changed to pwr4


================
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
+
----------------
hubert.reinterpretcast wrote:
> There's two spaces after the colon here (but one space after the colon on the previous lines).
changed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71551