Output alignment in zerofill and comm only if needed

[gchatelet]

Summary

13 years ago, the following commit changed the way alignment was represented moving from Log2 of alignment to plain number of bytes.
https://github.com/llvm/llvm-project/commit/6a715dccdf596d39d2b480beac48385f1fa73219#diff-d6520fbf2b954c8679d0835ca53c6ae423b4354e5e517dcfb043446042249f98

This patch introduced a regression, relevant lines are the following:
Before : if (Pow2Alignment != 0) OS << ',' << Pow2Alignment;
After : if (ByteAlignment != 0) OS << ',' << Log2_32(ByteAlignment);

This impacted the emitCommonSymbol and emitZeroFill functions. Functions added later on like emitLocalCommonSymbol indeed output the alignment only when it is greater than one. e.g.,
https://github.com/llvm/llvm-project/blob/10d183b889daab4512d476c1645d24d4e8946e8c/llvm/lib/MC/MCAsmStreamer.cpp#L997
https://github.com/llvm/llvm-project/blob/10d183b889daab4512d476c1645d24d4e8946e8c/llvm/lib/MC/MCAsmStreamer.cpp#L1059

This patch makes the behavior uniform for all sections and outputs alignment only when greater than one.

[courbet]

I'm not familiar with .comm directives. Do we document them somewhere in LLVM ?

All I could find is this and the doc for gnu as. The latter states that:

When using ELF, the .comm directive takes an optional third argument. This
is the desired alignment of the symbol, specified as a byte boundary (for
example, an alignment of 16 means that the least significant 4 bits of the
address should be zero). The alignment must be an absolute expression, and
it must be a power of two. If ld allocates uninitialized memory for the
common symbol, it will use the alignment when placing the symbol. If no
alignment is specified, as will set the alignment to the largest power of
two less than or equal to the size of the symbol, up to a maximum of 16.

This seems to imply that the directives generated by LLVM are incorrect, as we're emitting log2 values rather than alignment values. What did I miss ?

[gchatelet]

In D138778#3952734, @courbet wrote:

This seems to imply that the directives generated by LLVM are incorrect, as we're emitting log2 values rather than alignment values. What did I miss ?

AFAIU (I'm no expert here), we write log2 values only if getCOMMDirectiveAlignmentIsInBytes() returns false.
I found the following instances :

• llvm/lib/MC/MCAsmInfoCOFF.cpp : logic seems to depend on MinGW's version
• llvm/lib/MC/MCAsmInfoDarwin.cpp:66 : unconditionally set to false for Darwin
• llvm/lib/MC/MCAsmInfoXCOFF.cpp:52 : unconditionally set to false for XCOFF
• llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCAsmInfo.cpp:39 : unconditionally set to false for AMDGPU
• llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCAsmInfo.cpp:43 : unconditionally set to false for WebAssembly

@MaskRay WDYT?

[MaskRay]

Sorry for the belated response. This change looks wrong. Without the third argument, the alignment appears min(st_size, 16) on ELF in GNU as.

alignment=1 (st_value=1) COMMON symbols will get wrong alignments.

[gchatelet]

In D138778#3969603, @MaskRay wrote:

Sorry for the belated response. This change looks wrong. Without the third argument, the alignment appears min(st_size, 16) on ELF in GNU as.

alignment=1 (st_value=1) COMMON symbols will get wrong alignments.

Thx ! Is there a reason to differentiate between 0 and 1 here ? Would that be OK to output alignment unconditionally ?

edit: AFAICT the value 0 is never used. All tests pass with assert(ByteAlignment != 0); in both common and zerofill functions.

[MaskRay]

For the producer, it should be fine if the third argument is always produced. I think its existence may sometimes imply that the producer doesn't care about the alignment.
The patch as is doesn't seem improve things.

llvm/lib/MC/MCAsmStreamer.cpp

 }
 
 void MCAsmStreamer::emitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                                      unsigned ByteAlignment) {
   OS << "\t.comm\t";
   Symbol->print(OS, MAI);
   OS << ',' << Size;
 
-  if (ByteAlignment != 0) {
+  if (ByteAlignment > 1) {
     if (MAI->getCOMMDirectiveAlignmentIsInBytes())
       OS << ',' << ByteAlignment;
     else
       OS << ',' << Log2_32(ByteAlignment);
   }
   EmitEOL();
 
   // Print symbol's rename (original name contains invalid character(s)) if
void MCAsmStreamer::emitZerofill(MCSection *Section, MCSymbol *Symbol,
 
   const MCSectionMachO *MOSection = ((const MCSectionMachO*)Section);
   OS << MOSection->getSegmentName() << "," << MOSection->getName();
 
   if (Symbol) {
     OS << ',';
     Symbol->print(OS, MAI);
     OS << ',' << Size;
-    if (ByteAlignment != 0)
+    if (ByteAlignment > 1)
       OS << ',' << Log2_32(ByteAlignment);
   }
   EmitEOL();
 }
 
 // .tbss sym, size, align
 // This depends that the symbol has already been mangled from the original,
 // e.g. _a.

llvm/test/CodeGen/ARM/2010-12-15-elf-lcomm.ll

 ; RUN: llc  %s -mtriple=armv7-linux-gnueabi -filetype=obj -o - | \
 ; RUN:    llvm-readobj -S --symbols - | FileCheck  -check-prefix=OBJ %s
 ; RUN: llc  %s -mtriple=armv7-linux-gnueabi -o - | \
 ; RUN:    FileCheck  -check-prefix=ASM %s
 
 
 @dummy = internal global i32 666
 @array00 = internal global [80 x i8] zeroinitializer, align 1
 @sum = internal global i32 55
 @STRIDE = internal global i32 8
 
 ; ASM:          .type   array00,%object         @ @array00
 ; ASM-NEXT:     .local  array00
-; ASM-NEXT:     .comm   array00,80,1
+; ASM-NEXT:     .comm   array00,80
 ; ASM-NEXT:     .type   sum,%object  @ @sum
 
 
 ; OBJ:      Symbols [
 ; OBJ:        Symbol {
 ; OBJ:          Name: array00
 ; OBJ-NEXT:     Value: 0x0
 ; OBJ-NEXT:     Size: 80

llvm/test/CodeGen/ARM/elf-lcomm-align.ll

 ; RUN: llc < %s -mtriple=arm-linux-gnueabi -O0 | FileCheck %s
 ; run with -O0 to avoid arm global merging.
 
 @c = internal global i8 0, align 1
 @x = internal global i32 0, align 4
 
 ; .lcomm doesn't support alignment, so we always use .local/.comm.
 ; CHECK: .local c
-; CHECK-NEXT: .comm c,1,1
+; CHECK-NEXT: .comm c,1
 ; CHECK: .local x
 ; CHECK-NEXT: .comm x,4,4
 
 define i32 @foo() nounwind {
   ret i32 sub (i32 ptrtoint (i8* @c to i32), i32 ptrtoint (i32* @x to i32))
 }

llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll

 ; CHECK32-NEXT:     .vbyte  4, 0
 ; CHECK64-NEXT:     .vbyte  8, 0x401c000000000000
 ; CHECK-NEXT:       .vbyte  4, 1
 ; CHECK-NEXT:       .space  4
 
 
 ; CHECK-NEXT: .comm   a[RW],4,2
 ; CHECK-NEXT: .comm   b[RW],8,3
-; CHECK-NEXT: .comm   c[RW],2,1
+; CHECK-NEXT: .comm   c[RW],2
 ; CHECK-NEXT: .comm   d[RW],8,3
 ; CHECK-NEXT: .comm   f[RW],4,2
 ; CHECK-NEXT: .comm   over_aligned_comm[RW],8,5
-; CHECK-NEXT: .comm   array[RW],33,0
+; CHECK-NEXT: .comm   array[RW],33
 
 ; OBJ:      File: {{.*}}aix-xcoff-data.ll.tmp.o
 ; OBJ-NEXT: Format: aixcoff-rs6000
 ; OBJ-NEXT: Arch: powerpc
 ; OBJ-NEXT: AddressSize: 32bit
 ; OBJ-NEXT: FileHeader {
 ; OBJ-NEXT:   Magic: 0x1DF
 ; OBJ-NEXT:   NumberOfSections: 3

llvm/test/CodeGen/X86/global-sections.ll

 ; WIN32-SECTIONS: .section      .rdata,"dr",one_only,_G16
 ; WIN32-SECTIONS: _G16:
 
 ; PR26570
 
 @G17 = internal global i8 0
 ; LINUX: .type  G17,@object
 ; LINUX: .local G17
-; LINUX: .comm  G17,1,1
+; LINUX: .comm  G17,1
 
-; DARWIN: .zerofill __DATA,__bss,_G17,1,0
+; DARWIN: .zerofill __DATA,__bss,_G17,1
 
 ; LINUX-SECTIONS: .type G17,@object
 ; LINUX-SECTIONS: .section  .bss.G17,"aw",@nobits
 ; LINUX-SECTIONS: .byte 0
 ; LINUX-SECTIONS: .size G17, 1
 
 ; WIN32-SECTIONS: .section  .bss,"bw",one_only,_G17
 ; WIN32-SECTIONS: _G17:
 ; WIN32-SECTIONS:.byte  0
 
 ; check weak ReadOnlyWithRel globals.
 @G18 = linkonce_odr unnamed_addr constant ptr @G15
 ; DARWIN64: .section      __DATA,__const
 ; DARWIN64: _G18:

llvm/test/MC/COFF/comm.ll

 ; RUN: llc -mtriple i386-pc-mingw32 < %s | FileCheck %s
 
 @a = internal global i8 0, align 1
 @b = internal global double 0.000000e+00, align 8
 @c = common global i8 0, align 1
 @d = common global double 0.000000e+00, align 8
 
 ; .lcomm uses byte alignment
 ; CHECK: .lcomm _a,1
 ; CHECK: .lcomm _b,8,8
 ; .comm uses log2 alignment
-; CHECK: .comm  _c,1,0
+; CHECK: .comm  _c,1
 ; CHECK: .comm  _d,8,3