[MC] Omit fill value if it's zero when emitting code alignment

[steplong]

Summary

Previously, we were generating zeroes when generating code alignments for AArch64, but now we should omit the value and let the assembler choose to generate nops or zeroes.

[efriedma]

Reinterpreting Value == 0 to mean the user didn't specify a value is wrong. We should distinguish between "no fill specified" and "fill zero specified". (At least, that's how it should work, as far as I can tell.)

In the "no fill specified" case, we should end up in emitCodeAlignment; we can emit the correct directive from there, instead of throwing away the information.

[steplong]

In D132508#3744078, @efriedma wrote:

Reinterpreting Value == 0 to mean the user didn't specify a value is wrong. We should distinguish between "no fill specified" and "fill zero specified". (At least, that's how it should work, as far as I can tell.)

In the "no fill specified" case, we should end up in emitCodeAlignment; we can emit the correct directive from there, instead of throwing away the information.

That makes sense. Do you know how I can get the min byte width of the Value? The nop value for ARM64 is 0xd503201f which is 4 bytes, but X86's nop value is 0x90 which is 1 byte. Currently emitCodeAlignment looks like:

void emitValueToAlignment(unsigned ByteAlignment, int64_t Value = 0,
                          unsigned ValueSize = 1,
                          unsigned MaxBytesToEmit = 0) override;

void MCAsmStreamer::emitCodeAlignment(unsigned ByteAlignment,
                                      const MCSubtargetInfo *STI,
                                      unsigned MaxBytesToEmit) {
  // Emit with a text fill value.
  emitValueToAlignment(ByteAlignment, MAI->getTextAlignFillValue(),
                       1, MaxBytesToEmit);
}

ValueSize is hardcoded to 1, but for ARM64 we would have to pass 4.

[steplong]

• Update text fill value to nop instruction for AArch64
• Still need to set ValueSize arg to 4 for AArch64

[steplong]

• Omit value if value is 0 when emitting code alignment

[efriedma]

llvm/lib/MC/MCAsmStreamer.cpp

248

The name "emitValueToAlignmentIfNotOmit" doesn't make any sense; the "IfNotOmit" part isn't related to the rest of the name. Maybe just "emitAlignmentDirective"?

251

The logic around OmitValueIfZero is sort of hard to understand. Maybe pass in an Optional<int64_t> Value, and make the caller figure out whether the value should be omitted?

[steplong]

• Change logic to use Optional
• Rename to emitAlignmentDirective

[efriedma]

llvm/lib/MC/MCAsmStreamer.cpp

1466

else if (MaxBytesToEmit)?

1473

If I'm understand correctly, this is actually a visible change to emitValueToAlignment: it will always explicitly emit the value, even if it's zero. That's probably what we want (reducing ambiguity in our asm output seems like an improvement), but we should have test coverage for the change.

[steplong]

• Add test for .p2align NUM, 0x0 case
• Add missing if check for MaxBytesToEmit

[efriedma]

LGTM

[MaskRay]

LGTM.

llvm/test/CodeGen/AArch64/p2align-zero-fillvalue.ll

1 ↗

(On Diff #455369)

Use aarch64 to test generic ELF behavior, not just linux-gnu.

8 ↗

(On Diff #455369)

Add trailing {{$}}

[steplong]

• Fix failing tests
• Fixup p2align-zero-fillvalue.ll test

[steplong]

• Whoops missed a failing test

llvm/lib/MC/MCAsmStreamer.cpp

public:
   void emitGPRel32Value(const MCExpr *Value) override;
 
   void emitFill(const MCExpr &NumBytes, uint64_t FillValue,
                 SMLoc Loc = SMLoc()) override;
 
   void emitFill(const MCExpr &NumValues, int64_t Size, int64_t Expr,
                 SMLoc Loc = SMLoc()) override;
 
+  void emitValueToAlignmentIfNotOmit(unsigned ByteAlignment, int64_t Value,
+                                     unsigned ValueSize,
+                                     unsigned MaxBytesToEmit,
+                                     bool OmitValueIfZero);
+
   void emitValueToAlignment(unsigned ByteAlignment, int64_t Value = 0,
                             unsigned ValueSize = 1,
                             unsigned MaxBytesToEmit = 0) override;
 
   void emitCodeAlignment(unsigned ByteAlignment, const MCSubtargetInfo *STI,
                          unsigned MaxBytesToEmit = 0) override;
 
   void emitValueToOffset(const MCExpr *Offset,
void MCAsmStreamer::emitFill(const MCExpr &NumValues, int64_t Size,
   // FIXME: Emit location directives
   OS << "\t.fill\t";
   NumValues.print(OS, MAI);
   OS << ", " << Size << ", 0x";
   OS.write_hex(truncateToSize(Expr, 4));
   EmitEOL();
 }
 
-void MCAsmStreamer::emitValueToAlignment(unsigned ByteAlignment, int64_t Value,
+void MCAsmStreamer::emitValueToAlignmentIfNotOmit(unsigned ByteAlignment,
+                                                  int64_t Value,
                                                   unsigned ValueSize,
-                                         unsigned MaxBytesToEmit) {
+                                                  unsigned MaxBytesToEmit,
+                                                  bool OmitValueIfZero) {
   if (MAI->useDotAlignForAlignment()) {
     if (!isPowerOf2_32(ByteAlignment))
       report_fatal_error("Only power-of-two alignments are supported "
                          "with .align.");
     OS << "\t.align\t";
     OS << Log2_32(ByteAlignment);
     EmitEOL();
     return;
case 4:
       break;
     case 8:
       llvm_unreachable("Unsupported alignment size!");
     }
 
     OS << Log2_32(ByteAlignment);
 
     if (Value || MaxBytesToEmit) {
+      if (Value || !OmitValueIfZero) {
         OS << ", 0x";
         OS.write_hex(truncateToSize(Value, ValueSize));
+      } else if (!Value && OmitValueIfZero && MaxBytesToEmit) {
+        OS << ", ";
+      }
 
       if (MaxBytesToEmit)
         OS << ", " << MaxBytesToEmit;
     }
     EmitEOL();
     return;
   }
 
   // Non-power of two alignment.  This is not widely supported by assemblers.
   // FIXME: Parameterize this based on MAI.
   switch (ValueSize) {
   default: llvm_unreachable("Invalid size for machine code value!");
   case 1: OS << ".balign";  break;
   case 2: OS << ".balignw"; break;
   case 4: OS << ".balignl"; break;
   case 8: llvm_unreachable("Unsupported alignment size!");
   }
 
   OS << ' ' << ByteAlignment;
   OS << ", " << truncateToSize(Value, ValueSize);
   if (MaxBytesToEmit)
     OS << ", " << MaxBytesToEmit;
   EmitEOL();
 }
 
+void MCAsmStreamer::emitValueToAlignment(unsigned ByteAlignment, int64_t Value,
+                                         unsigned ValueSize,
+                                         unsigned MaxBytesToEmit) {
+  emitValueToAlignmentIfNotOmit(ByteAlignment, Value, ValueSize, MaxBytesToEmit,
+                                false);
+}
+
 void MCAsmStreamer::emitCodeAlignment(unsigned ByteAlignment,
                                       const MCSubtargetInfo *STI,
                                       unsigned MaxBytesToEmit) {
   // Emit with a text fill value.
-  emitValueToAlignment(ByteAlignment, MAI->getTextAlignFillValue(),
-                       1, MaxBytesToEmit);
+  emitValueToAlignmentIfNotOmit(ByteAlignment, MAI->getTextAlignFillValue(), 1,
+                                MaxBytesToEmit, true);
 }
 
 void MCAsmStreamer::emitValueToOffset(const MCExpr *Offset,
                                       unsigned char Value,
                                       SMLoc Loc) {
   // FIXME: Verify that Offset is associated with the current section.
   OS << ".org ";
   Offset->print(OS, MAI);

llvm/test/CodeGen/AArch64/aarch64-p2align-max-bytes-neoverse.ll

 ; RUN: llc -mtriple=aarch64-none-linux-gnu -align-loops=32   < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-DEFAULT
 ; RUN: llc -mtriple=aarch64-none-linux-gnu -mcpu=neoverse-n1 < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-16
 ; RUN: llc -mtriple=aarch64-none-linux-gnu -mcpu=neoverse-n2 < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-16
 ; RUN: llc -mtriple=aarch64-none-linux-gnu -mcpu=neoverse-v1 < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-16
 ; RUN: llc -mtriple=aarch64-none-linux-gnu -mcpu=cortex-x1   < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-16
 ; RUN: llc -mtriple=aarch64-none-linux-gnu -mcpu=cortex-x2   < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-16
 ; RUN: llc -mtriple=aarch64-none-linux-gnu -mcpu=cortex-a35  < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-8
 ; RUN: llc -mtriple=aarch64-none-linux-gnu -mcpu=cortex-a53  < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-8
 ; RUN: llc -mtriple=aarch64-none-linux-gnu -mcpu=cortex-a55  < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-8
 ; RUN: llc -mtriple=aarch64-none-linux-gnu -mcpu=cortex-a57  < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-8
 ; RUN: llc -mtriple=aarch64-none-linux-gnu -mcpu=cortex-a510 < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-8
 ; RUN: llc -mtriple=aarch64-none-linux-gnu -mcpu=cortex-a75  < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-8
 ; RUN: llc -mtriple=aarch64-none-linux-gnu -mcpu=cortex-a710 < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-16
 
 define i32 @a(i32 %x, i32* nocapture readonly %y, i32* nocapture readonly %z) {
 ; CHECK-DEFAULT:    .p2align 5
-; CHECK-8:          .p2align 4, 0x0, 8
-; CHECK-16:         .p2align 5, 0x0, 16
+; CHECK-8:          .p2align 4, , 8
+; CHECK-16:         .p2align 5, , 16
 ; CHECK-NEXT:       .LBB0_5: // %vector.body
 ; CHECK-DEFAULT:    .p2align 5
-; CHECK-8:          .p2align 4, 0x0, 8
-; CHECK-16:         .p2align 5, 0x0, 16
+; CHECK-8:          .p2align 4, , 8
+; CHECK-16:         .p2align 5, , 16
 ; CHECK-NEXT:       .LBB0_8: // %for.body
 entry:
   %cmp10 = icmp sgt i32 %x, 0
   br i1 %cmp10, label %for.body.preheader, label %for.cond.cleanup
 
 for.body.preheader:                               ; preds = %entry
   %wide.trip.count = zext i32 %x to i64
   %min.iters.check = icmp ult i32 %x, 8

llvm/test/CodeGen/AArch64/aarch64-p2align-max-bytes.ll

 ; RUN: llc -mtriple=aarch64-none-linux-gnu -max-bytes-for-alignment=8 --align-loops=32 < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-EXPLICIT
 ; RUN: llc -mtriple=aarch64-none-linux-gnu --align-loops=32 < %s -o -| FileCheck %s --check-prefixes=CHECK,CHECK-IMPLICIT
 ; RUN: llc -mtriple=aarch64-none-linux-gnu --align-loops=32 < %s -o - --filetype=obj | llvm-objdump --arch=aarch64  -d -| FileCheck %s --check-prefixes=CHECK-OBJ,CHECK-OBJ-IMPLICIT
 ; RUN: llc -mtriple=aarch64-none-linux-gnu -max-bytes-for-alignment=8 --align-loops=32 < %s -o - --filetype=obj | llvm-objdump --arch=aarch64  -d -| FileCheck %s --check-prefixes=CHECK-OBJ,CHECK-OBJ-EXPLICIT
 
 ; This test is checking that the correct operands to the .p2align are emitted correctly, and that the resulting obj
 ; is padded as expected. The key interest in the CHECK-OBJ-* sections is the size of the padding region (the nops),
 ; and not the exact instructions either side of them (But the last instruction of the EXPLICIT and IMPLICIT checks
 ; should be the same, at different locations)
 define i32 @a(i32 %x, i32* nocapture readonly %y, i32* nocapture readonly %z) {
 ; CHECK-LABEL: a:
-; CHECK-EXPLICIT:    .p2align 5, 0x0, 8
+; CHECK-EXPLICIT:    .p2align 5, , 8
 ; CHECK-IMPLICIT:    .p2align 5
 ; CHECK-NEXT:  .LBB0_5: // %vector.body
-; CHECK-EXPLICIT:    .p2align 5, 0x0, 8
+; CHECK-EXPLICIT:    .p2align 5, , 8
 ; CHECK-IMPLICIT:    .p2align 5
 ; CHECK-NEXT:  .LBB0_8: // %for.body
 ; CHECK-OBJ;Disassembly of section .text:
 ; CHECK-OBJ:               88: 8b0a002a      add
 ; CHECK-OBJ-IMPLICIT-NEXT: 8c: d503201f      nop
 ; CHECK-OBJ-IMPLICIT-NEXT: 90: d503201f      nop
 ; CHECK-OBJ-IMPLICIT-NEXT: 94: d503201f      nop
 ; CHECK-OBJ-IMPLICIT-NEXT: 98: d503201f      nop

llvm/test/CodeGen/AArch64/aarch64-sched-store.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple aarch64 -mcpu=cortex-a57 -mattr=+slow-paired-128 < %s | FileCheck %s --check-prefixes=DEFAULT
 ; RUN: llc -mtriple aarch64 -mcpu=cortex-a57 -mattr=+slow-paired-128 -mattr=+ascend-store-address < %s | FileCheck %s --check-prefixes=ASCEND
 
 target triple = "aarch64-unknown-linux-gnu"
 
 define dso_local void @memset_unroll2(double* nocapture %array, i64 %size) {
 ; DEFAULT-LABEL: memset_unroll2:
 ; DEFAULT:       // %bb.0: // %entry
 ; DEFAULT-NEXT:    fmov v0.2d, #2.00000000
 ; DEFAULT-NEXT:    add x8, x0, #64
-; DEFAULT-NEXT:    .p2align 4, 0x0, 8
+; DEFAULT-NEXT:    .p2align 4, , 8
 ; DEFAULT-NEXT:  .LBB0_1: // %vector.body
 ; DEFAULT-NEXT:    // =>This Inner Loop Header: Depth=1
 ; DEFAULT-NEXT:    stur q0, [x8, #-64]
 ; DEFAULT-NEXT:    subs x1, x1, #4
 ; DEFAULT-NEXT:    stur q0, [x8, #-48]
 ; DEFAULT-NEXT:    str q0, [x8]
 ; DEFAULT-NEXT:    str q0, [x8, #16]
 ; DEFAULT-NEXT:    str q0, [x8, #32]
 ; DEFAULT-NEXT:    str q0, [x8, #48]
 ; DEFAULT-NEXT:    stur q0, [x8, #-32]
 ; DEFAULT-NEXT:    stur q0, [x8, #-16]
 ; DEFAULT-NEXT:    add x8, x8, #128
 ; DEFAULT-NEXT:    b.ne .LBB0_1
 ; DEFAULT-NEXT:  // %bb.2: // %cleanup
 ; DEFAULT-NEXT:    ret
 ;
 ; ASCEND-LABEL: memset_unroll2:
 ; ASCEND:       // %bb.0: // %entry
 ; ASCEND-NEXT:    fmov v0.2d, #2.00000000
 ; ASCEND-NEXT:    add x8, x0, #64
-; ASCEND-NEXT:    .p2align 4, 0x0, 8
+; ASCEND-NEXT:    .p2align 4, , 8
 ; ASCEND-NEXT:  .LBB0_1: // %vector.body
 ; ASCEND-NEXT:    // =>This Inner Loop Header: Depth=1
 ; ASCEND-NEXT:    stur q0, [x8, #-64]
 ; ASCEND-NEXT:    subs x1, x1, #4
 ; ASCEND-NEXT:    stur q0, [x8, #-48]
 ; ASCEND-NEXT:    stur q0, [x8, #-32]
 ; ASCEND-NEXT:    stur q0, [x8, #-16]
 ; ASCEND-NEXT:    str q0, [x8]

llvm/test/CodeGen/AArch64/merge-store-dependency.ll

 ; A53-NEXT:    bl fcntl
 ; A53-NEXT:    adrp x9, gv0
 ; A53-NEXT:    add x9, x9, :lo12:gv0
 ; A53-NEXT:    cmp x19, x9
 ; A53-NEXT:    b.eq .LBB0_4
 ; A53-NEXT:  // %bb.1:
 ; A53-NEXT:    ldr w8, [x19]
 ; A53-NEXT:    ldr w9, [x9]
-; A53-NEXT:    .p2align 4, 0x0, 8
+; A53-NEXT:    .p2align 4, , 8
 ; A53-NEXT:  .LBB0_2: // %while.body.i.split.ver.us
 ; A53-NEXT:    // =>This Inner Loop Header: Depth=1
 ; A53-NEXT:    lsl w9, w9, #1
 ; A53-NEXT:    cmp w9, w8
 ; A53-NEXT:    b.le .LBB0_2
 ; A53-NEXT:  // %bb.3: // %while.end.i
 ; A53-NEXT:    bl foo
 ; A53-NEXT:    adrp x8, gv1
 ; A53-NEXT:    str x0, [x8, :lo12:gv1]
 ; A53-NEXT:    ldp x30, x19, [sp], #16 // 16-byte Folded Reload
 ; A53-NEXT:    .cfi_def_cfa_offset 0
 ; A53-NEXT:    .cfi_restore w19
 ; A53-NEXT:    .cfi_restore w30
 ; A53-NEXT:    ret
-; A53-NEXT:    .p2align 4, 0x0, 8
+; A53-NEXT:    .p2align 4, , 8
 ; A53-NEXT:  .LBB0_4: // %while.body.i.split
 ; A53-NEXT:    // =>This Inner Loop Header: Depth=1
 ; A53-NEXT:    .cfi_restore_state
 ; A53-NEXT:    b .LBB0_4
 entry:
   %0 = bitcast %struct1* %fde to i8*
   tail call void @llvm.memset.p0i8.i64(i8* align 8 %0, i8 0, i64 40, i1 false)
   %state = getelementptr inbounds %struct1, %struct1* %fde, i64 0, i32 4