[MC] Make bundle alignment mode setting idempotent and support nested bundles

[dschuff]

Summary

Currently an error is thrown if bundle alignment mode is set more than once
per module (either via the API or the .bundle_align_mode directive). This
change allows setting it multiple times as long as the alignment doesn't
change.

Also nested bundle_lock groups are currently not allowed. This change allows
them, with the effect that the group stays open until all nests are exited,
and if any of the bundle_lock directives has the align_to_end flag, the
group becomes align_to_end.

These changes make the bundle aligment simpler to use in the compiler, and
also better match the corresponding support in GNU as.

[eliben]

LGTM, with a couple of nits

lib/MC/MCAssembler.cpp

338 ↗

(On Diff #14947)

if (

346 ↗

(On Diff #14947)

Can you put {...} around the contents, for consistency with other code in this method?

lib/MC/MCELFStreamer.cpp

481 ↗

(On Diff #14947)

s/1/1U/ for consistency with above?

[dschuff]

• Reviewer comments

[jvoung]

otherwise LGTM too

lib/MC/MCELFStreamer.cpp

483 ↗

(On Diff #14952)

Would be good add a test that the error is thrown when the align modes are changed.

test/MC/X86/AlignedBundling/nesting.s

11 ↗

(On Diff #14952)

isntructions -> instructions

56 ↗

(On Diff #14952)

No need to say callq is 5 bytes long here too (already stated earlier)

[dschuff]

• jvoung review comments

[dschuff]

Closed by commit rL219811 (authored by @dschuff).

llvm/trunk/include/llvm/MC/MCAssembler.h

private:
 
   /// LayoutOrder - The index of this section in the layout order.
   unsigned LayoutOrder;
 
   /// Alignment - The maximum alignment seen in this section.
   unsigned Alignment;
 
   /// \brief Keeping track of bundle-locked state.
-  BundleLockStateType BundleLockState; 
+  BundleLockStateType BundleLockState;
 
+  /// \brief Current nesting depth of bundle_lock directives.
+  unsigned BundleLockNestingDepth;
+
   /// \brief We've seen a bundle_lock directive but not its first instruction
   /// yet.
   bool BundleGroupBeforeFirstInst;
 
   /// @name Assembler Backend Data
   /// @{
   //
   // FIXME: This could all be kept private to the assembler implementation.
public:
   bool isBundleLocked() const {
     return BundleLockState != NotBundleLocked;
   }
 
   BundleLockStateType getBundleLockState() const {
     return BundleLockState;
   }
 
-  void setBundleLockState(BundleLockStateType NewState) {
-    BundleLockState = NewState;
-  }
+  void setBundleLockState(BundleLockStateType NewState);
 
   bool isBundleGroupBeforeFirstInst() const {
     return BundleGroupBeforeFirstInst;
   }
 
   void setBundleGroupBeforeFirstInst(bool IsFirst) {
     BundleGroupBeforeFirstInst = IsFirst;
   }

llvm/trunk/lib/MC/MCAssembler.cpp

 /* *** */
 
 MCSectionData::MCSectionData() : Section(nullptr) {}
 
 MCSectionData::MCSectionData(const MCSection &_Section, MCAssembler *A)
   : Section(&_Section),
     Ordinal(~UINT32_C(0)),
     Alignment(1),
-    BundleLockState(NotBundleLocked), BundleGroupBeforeFirstInst(false),
+    BundleLockState(NotBundleLocked),
+    BundleLockNestingDepth(0),
+    BundleGroupBeforeFirstInst(false),
     HasInstructions(false)
 {
   if (A)
     A->getSectionList().push_back(this);
 }
 
 MCSectionData::iterator
 MCSectionData::getSubsectionInsertionPoint(unsigned Subsection) {
if (!ExactMatch && Subsection != 0) {
     MCFragment *F = new MCDataFragment();
     SubsectionFragmentMap.insert(MI, std::make_pair(Subsection, F));
     getFragmentList().insert(IP, F);
     F->setParent(this);
   }
   return IP;
 }
 
+void MCSectionData::setBundleLockState(BundleLockStateType NewState) {
+  if (NewState == NotBundleLocked) {
+    if (BundleLockNestingDepth == 0) {
+      report_fatal_error("Mismatched bundle_lock/unlock directives");
+    }
+    if (--BundleLockNestingDepth == 0) {
+      BundleLockState = NotBundleLocked;
+    }
+    return;
+  }
+
+  // If any of the directives is an align_to_end directive, the whole nested
+  // group is align_to_end. So don't downgrade from align_to_end to just locked.
+  if (BundleLockState != BundleLockedAlignToEnd) {
+    BundleLockState = NewState;
+  }
+  ++BundleLockNestingDepth;
+}
+
 /* *** */
 
 MCSymbolData::MCSymbolData() : Symbol(nullptr) {}
 
 MCSymbolData::MCSymbolData(const MCSymbol &_Symbol, MCFragment *_Fragment,
                            uint64_t _Offset, MCAssembler *A)
     : Symbol(&_Symbol), Fragment(_Fragment), Offset(_Offset),
       SymbolSize(nullptr), CommonAlign(-1U), Flags(0), Index(0) {

llvm/trunk/lib/MC/MCELFStreamer.cpp

else if (!SD->isBundleLocked() && Fixups.size() == 0) {
       // there are no fixups registered.
       MCCompactEncodedInstFragment *CEIF = new MCCompactEncodedInstFragment();
       insert(CEIF);
       CEIF->getContents().append(Code.begin(), Code.end());
       return;
     } else {
       DF = new MCDataFragment();
       insert(DF);
-      if (SD->getBundleLockState() == MCSectionData::BundleLockedAlignToEnd) {
-        // If this is a new fragment created for a bundle-locked group, and the
-        // group was marked as "align_to_end", set a flag in the fragment.
-        DF->setAlignToBundleEnd(true);
-      }
-    }
+    }
+      // If this fragment is for a group marked "align_to_end", set a flag
+      // in the fragment. This can happen after the fragment has already been
+      // created if there are nested bundle_align groups and an inner one
+      // is the one marked align_to_end.
 
     // We're now emitting an instruction in a bundle group, so this flag has
     // to be turned off.
     SD->setBundleGroupBeforeFirstInst(false);
   } else {
     DF = getOrCreateDataFragment();
   }
 
   // Add the fixups and data.
   for (unsigned i = 0, e = Fixups.size(); i != e; ++i) {
     Fixups[i].setOffset(Fixups[i].getOffset() + DF->getContents().size());
     DF->getFixups().push_back(Fixups[i]);
   }
   DF->setHasInstructions(true);
   DF->getContents().append(Code.begin(), Code.end());
 }
 
 void MCELFStreamer::EmitBundleAlignMode(unsigned AlignPow2) {
   assert(AlignPow2 <= 30 && "Invalid bundle alignment");
   MCAssembler &Assembler = getAssembler();
-  if (Assembler.getBundleAlignSize() == 0 && AlignPow2 > 0)
-    Assembler.setBundleAlignSize(1 << AlignPow2);
+  if (AlignPow2 > 0 && (Assembler.getBundleAlignSize() == 0 ||
+                        Assembler.getBundleAlignSize() == 1U << AlignPow2))
+    Assembler.setBundleAlignSize(1U << AlignPow2);
   else
-    report_fatal_error(".bundle_align_mode should be only set once per file");
+    report_fatal_error(".bundle_align_mode cannot be changed once set");
 }
 
 void MCELFStreamer::EmitBundleLock(bool AlignToEnd) {
   MCSectionData *SD = getCurrentSectionData();
 
   // Sanity checks
   //
   if (!getAssembler().isBundlingEnabled())
     report_fatal_error(".bundle_lock forbidden when bundling is disabled");
-  else if (SD->isBundleLocked())
-    report_fatal_error("Nesting of .bundle_lock is forbidden");
+
+  if (!SD->isBundleLocked())
+    SD->setBundleGroupBeforeFirstInst(true);
 
   SD->setBundleLockState(AlignToEnd ? MCSectionData::BundleLockedAlignToEnd :
                                       MCSectionData::BundleLocked);
-  SD->setBundleGroupBeforeFirstInst(true);
 }
 
 void MCELFStreamer::EmitBundleUnlock() {
   MCSectionData *SD = getCurrentSectionData();
 
   // Sanity checks
   if (!getAssembler().isBundlingEnabled())
     report_fatal_error(".bundle_unlock forbidden when bundling is disabled");

llvm/trunk/test/MC/X86/AlignedBundling/nesting.s

+# RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - \
+# RUN:   | llvm-objdump -disassemble -no-show-raw-insn - | FileCheck %s
+
+# Will be bundle-aligning to 16 byte boundaries
+  .bundle_align_mode 4
+  .text
+# CHECK-LABEL: foo
+foo:
+# Test that bundle alignment mode can be set more than once.
+  .bundle_align_mode 4
+# Each of these callq instructions is 5 bytes long
+  callq bar
+  callq bar
+  .bundle_lock
+  .bundle_lock
+  callq bar
+  callq bar     
+  .bundle_unlock
+  .bundle_unlock
+# CHECK:      10: callq
+# CHECK-NEXT: 15: callq
+
+  .p2align 4
+# CHECK-LABEL: bar
+bar:
+  callq foo
+  callq foo
+# Check that the callqs get bundled together, and that the whole group is
+# align_to_end
+  .bundle_lock 
+  callq bar
+  .bundle_lock align_to_end
+  callq bar
+  .bundle_unlock
+  .bundle_unlock
+# CHECK:      36: callq
+# CHECK-NEXT: 3b: callq
+
+# CHECK-LABEL: baz
+baz:
+  callq foo
+  callq foo
+# Check that the callqs get bundled together, and that the whole group is
+# align_to_end (with the outer directive marked align_to_end)
+  .bundle_lock align_to_end
+  callq bar
+  .bundle_lock
+  callq bar
+  .bundle_unlock
+  .bundle_unlock
+# CHECK:      56: callq
+# CHECK-NEXT: 5b: callq
+
+# CHECK-LABEL: quux
+quux:
+  callq bar
+  callq bar
+  .bundle_lock
+  .bundle_lock
+  callq bar
+  .bundle_unlock
+  callq bar     
+  .bundle_unlock
+# Check that the calls are bundled together when the second one is after the
+# inner nest is closed.
+# CHECK:      70: callq
+# CHECK-NEXT: 75: callq