[MC] Align fragments when -mc-relax-all flag is used

[phosek]

Summary

Ensure that fragments are bundle aligned when instruction bundling
is enabled and the -mc-relax-all flag is set. This is implicitly
assumed by the bundle padding implementation but this assumption
does not hold when custom alignment is being used.

The change was tested by running PNaCl toolchain trybots with
-mc-relax-all flag set.

Fixes https://code.google.com/p/nativeclient/issues/detail?id=4063

[mseaborn]

Can you add a bit more context in the commit message:

• Link to the NaCl bug URL.
• Say that this is fixing a regression from the earlier "-mc-relax-all" change, which caused ".align" to produce non-validating output. Are there any other directives that output new fragments that this change would fix?

Also, can you say how you tested this, please? Did you run the PNaCl toolchain trybots with "-mc-relax-all" added in pnacl-translate.py?

lib/MC/MCAssembler.cpp

259 ↗

(On Diff #26549)

Can you explain this part of the change in the commit message?

Presumably, without this change, ".align 16" would cause an extra 32 bytes of no-ops to be added?

589 ↗

(On Diff #26549)

How about adding more context? i.e.:

"""
Without -mc-relax-all, we emit one instruction per fragment. These fragments don't contain no-op bundle padding, so the fragments make no assumption about their alignment.

With -mc-relax-all, we emit multiple instructions per fragment, with no-op bundle padding inside fragments, which means these fragments potentially assume they are bundle-aligned. (At least, a fragment that is larger than the bundle size will assume it is bundle-aligned.) This means that if we end up with multiple fragments, we must emit bundle padding between fragments.

".align N" is an example of a directive that introduces multiple fragments. It might be better to add a special case to handle ".align N" by emitting within-fragment padding (which would produce less padding when N is less than the bundle size), but for now we don't.
"""

test/MC/X86/AlignedBundling/misaligned-bundle-group.s

17 ↗

(On Diff #26549)

How about using "call" as a more realistic example, since that is what's used with align_to_end?

test/MC/X86/AlignedBundling/misaligned-bundle.s

13 ↗

(On Diff #26549)

It's quite difficult to tell, by reading this test, what it's intended to be testing for.

For a start, how about commenting how many bytes these instructions are meant to be?
e.g.
movl $0x1, (%esp) // 7 bytes [or 5 bytes?]

How about checking for these instructions and their offsets in the output too? The same applies to the other test.

[phosek]

• Added comment to provide more context

[phosek]

lib/MC/MCAssembler.cpp

589 ↗

(On Diff #26549)

Done.

test/MC/X86/AlignedBundling/misaligned-bundle-group.s

17 ↗

(On Diff #26549)

Done.

test/MC/X86/AlignedBundling/misaligned-bundle.s

13 ↗

(On Diff #26549)

Done.

[mseaborn]

LGTM, thanks.

test/MC/X86/AlignedBundling/misaligned-bundle-group.s

2 ↗

(On Diff #27342)

Nit: Split line with another "\" to keep lines <=80 chars?

[phosek]

• Use proper line length

[phosek]

test/MC/X86/AlignedBundling/misaligned-bundle-group.s

2 ↗

(On Diff #27342)

Done.

[phosek]

• Use proper line length

[mseaborn]

LGTM again (this time with "accept revision" selected)

llvm/trunk/lib/MC/MCAssembler.cpp

if (F->alignToBundleEnd()) {
     // intentionally kept in its more explicit form for simplicity.
     if (EndOfFragment == BundleSize)
       return 0;
     else if (EndOfFragment < BundleSize)
       return BundleSize - EndOfFragment;
     else { // EndOfFragment > BundleSize
       return 2 * BundleSize - EndOfFragment;
     }
-  } else if (EndOfFragment > BundleSize)
+  } else if (OffsetInBundle > 0 && EndOfFragment > BundleSize)
     return BundleSize - OffsetInBundle;
   else
     return 0;
 }
 
 /* *** */
 
 void ilist_node_traits<MCFragment>::deleteNode(MCFragment *V) {
void MCAsmLayout::layoutFragment(MCFragment *F) {
   //                    ^
   //                    |
   //                    F->Offset
   //
   // The fragment's offset will point to after the padding, and its computed
   // size won't include the padding.
   //
   // When the -mc-relax-all flag is used, we optimize bundling by writting the
-  // bundle padding directly into fragments when the instructions are emitted
-  // inside the streamer.
+  // padding directly into fragments when the instructions are emitted inside
+  // the streamer. When the fragment is larger than the bundle size, we need to
+  // ensure that it's bundle aligned. This means that if we end up with
+  // multiple fragments, we must emit bundle padding between fragments.
   //
-  if (Assembler.isBundlingEnabled() && !Assembler.getRelaxAll() &&
-      F->hasInstructions()) {
+  // ".align N" is an example of a directive that introduces multiple
+  // fragments. We could add a special case to handle ".align N" by emitting
+  // within-fragment padding (which would produce less padding when N is less
+  // than the bundle size), but for now we don't.
+  //
+  if (Assembler.isBundlingEnabled() && F->hasInstructions()) {
     assert(isa<MCEncodedFragment>(F) &&
            "Only MCEncodedFragment implementations have instructions");
     uint64_t FSize = Assembler.computeFragmentSize(*this, *F);
 
-    if (FSize > Assembler.getBundleAlignSize())
+    if (!Assembler.getRelaxAll() && FSize > Assembler.getBundleAlignSize())
       report_fatal_error("Fragment can't be larger than a bundle size");
 
     uint64_t RequiredBundlePadding = computeBundlePadding(Assembler, F,
                                                           F->Offset, FSize);
     if (RequiredBundlePadding > UINT8_MAX)
       report_fatal_error("Padding cannot exceed 255 bytes");
     F->setBundlePadding(static_cast<uint8_t>(RequiredBundlePadding));
     F->Offset += RequiredBundlePadding;

llvm/trunk/test/MC/X86/AlignedBundling/misaligned-bundle-group.s

+# RUN: llvm-mc -filetype=obj -triple i686-pc-linux-gnu %s -o - \
+# RUN:   | llvm-objdump -disassemble -no-show-raw-insn - \
+# RUN:   | FileCheck -check-prefix=CHECK -check-prefix=CHECK-OPT %s
+# RUN: llvm-mc -filetype=obj -triple i686-pc-linux-gnu -mc-relax-all %s -o - \
+# RUN:   | llvm-objdump -disassemble -no-show-raw-insn - \
+# RUN:   | FileCheck -check-prefix=CHECK -check-prefix=CHECK-RELAX %s
+
+        .text
+foo:
+        .bundle_align_mode 5
+        push    %ebp # 1 byte
+        .align  16
+        .bundle_lock align_to_end
+# CHECK:            1:  nopw %cs:(%eax,%eax)
+# CHECK:            10: nopw %cs:(%eax,%eax)
+# CHECK-RELAX:      1f: nop
+# CHECK-RELAX:      20: nopw %cs:(%eax,%eax)
+# CHECK-RELAX:      2f: nopw %cs:(%eax,%eax)
+# CHECK-OPT:        1b: calll -4
+# CHECK-RELAX:      3b: calll -4
+        calll   bar # 5 bytes
+        .bundle_unlock
+        ret         # 1 byte

llvm/trunk/test/MC/X86/AlignedBundling/misaligned-bundle.s

+# RUN: llvm-mc -filetype=obj -triple i686-pc-linux-gnu %s -o - \
+# RUN:   | llvm-objdump -disassemble -no-show-raw-insn - \
+# RUN:   | FileCheck -check-prefix=CHECK -check-prefix=CHECK-OPT %s
+# RUN: llvm-mc -filetype=obj -triple i686-pc-linux-gnu -mc-relax-all %s -o - \
+# RUN:   | llvm-objdump -disassemble -no-show-raw-insn - \
+# RUN:   | FileCheck -check-prefix=CHECK -check-prefix=CHECK-RELAX %s
+
+        .text
+foo:
+        .bundle_align_mode 5
+        push    %ebp          # 1 byte
+        .align  16
+# CHECK:            1:  nopw %cs:(%eax,%eax)
+# CHECK-RELAX:      10: nopw %cs:(%eax,%eax)
+# CHECK-RELAX:      1f: nop
+# CHECK-OPT:        10: movl $1, (%esp)
+# CHECK-RELAX:      20: movl $1, (%esp)
+        movl $0x1, (%esp)     # 7 bytes
+        movl $0x1, (%esp)     # 7 bytes
+# CHECK-OPT:        1e: nop
+        movl $0x2, 0x1(%esp)  # 8 bytes
+        movl $0x2, 0x1(%esp)  # 8 bytes
+# CHECK-RELAX:      3e: nop
+# CHECK-RELAX:      40: movl $2, 1(%esp)
+        movl $0x2, 0x1(%esp)  # 8 bytes
+        movl $0x2, (%esp)     # 7 bytes
+# CHECK-OPT:        3f: nop
+# CHECK-OPT:        40: movl $3, (%esp)
+        movl $0x3, (%esp)     # 7 bytes
+        movl $0x3, (%esp)     # 7 bytes
+        ret