[X86] Not track size of the boudaryalign fragment during the layout

[skan]

Summary

Currently the boundaryalign fragment caches its size during the process
of layout and then it is relaxed and update the size in each iteration. This
behaviour is unnecessary and ugly.

[skan]

remove the unused data member Size for MCBoundaryAlignFragment

[skan]

Change a comment

[skan]

fix the error reported by clang-tidy

[MaskRay]

Thanks for deleting the Size member variable. It can avoid some cache invalidation problems. LGTM, but please wait for another pair of eyes.

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

445

easily during the layout process.

[MaskRay]

llvm/lib/MC/MCAssembler.cpp

366

auto -> int

368

Delete excess parentheses.

[skan]

delete excess parentheses

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

445

oh..I missed this comment. Currently there are many redundant boundary align fragments inserted, I will open another revison to fix that and of course the code change will cover this comment.

[MaskRay]

Thanks for deleting the Size member variable. It can avoid some cache invalidation problems. LGTM, but please wait for another pair of eyes.

Closed by commit rG2ac19feb1571: [X86] Not track size of the boudaryalign fragment during the layout (authored by skan). · Explain Why

For this case, it is likely that @reames will want to review the change, so I provided a qualified approval. From D71916 "LGTM - How a Patch Is Accepted":

If it is likely that others will want to review a recently-posted patch, especially if there might be objections, but no one else has done so yet, it is also polite to provide a qualified approval (e.g., "LGTM, but please wait for a couple of days in case others wish to review"). If approval is received very quickly, a patch author may also elect to wait before committing (and this is certainly considered polite for non-trivial patches). Especially given the global nature of our community, this waiting time should be at least 24 hours. Please also be mindful of weekends and major holidays.

The advice may apply to https://reviews.llvm.org/D75017 as well. If Phabricator is telling the truth, with all due respect, Annita has not ever committed to llvm-project and has not actively reviewed the assembly mitigations (there may be some internal review process but that is not visible to us). It was likely that someone would suggest some wording change, so it was not very appropriate to commit just 3 hours (BTW, 18:00~22:00 are not working hours in Pacific Time) after Annita approved the patch. Additionally, IIUC, we are not native English speakers😹 and it would be very helpful to get wording suggestions from others.

Last, from the thread https://lists.llvm.org/pipermail/llvm-dev/2020-January/137889.html . It may be helpful to include Reviewed By: ... in the git commit description.

[skan]

In D75404#1900225, @MaskRay wrote:

Thanks for deleting the Size member variable. It can avoid some cache invalidation problems. LGTM, but please wait for another pair of eyes.

Closed by commit rG2ac19feb1571: [X86] Not track size of the boudaryalign fragment during the layout (authored by skan). · Explain Why

For this case, it is likely that @reames will want to review the change, so I provided a qualified approval. From D71916 "LGTM - How a Patch Is Accepted":

Thanks for pointing out. I missed the comments "easily during the layout process." and "wait for another pair of eyes" unintentionally.

[skan]

Commit https://reviews.llvm.org/rG2ac19feb1571960b8e1479a451b45ab56da7034e causes some test case to run fail and was reverted by me. We need some fixes for this.

[skan]

In e-mail @reames wrote:

Er, I'm not sure this is correct. The concern is that boundary align requires the computation of the size of a fragment following the boundary align. Since the size may be influenced by the starting offset (e.g. align), the size of the boundary align must already be known. The circularity of the logic there seems likely to lead to issues, though I don't have a specific test case.

Though... I guess the way we use BA makes this "safe" as we know the instructions following must be either DataFragments or RelaxableFragments.

And unfortunately，I did find some test cases failed during runtime. I agree with reame, one difference between boundar align
and align fragment is that the size of boundary align depends on the following fragment while align not. Before finishLayout, we have to fix its size before we write boudary align and this has to be done in relaxBoundaryAlign. I feel sorry that D75203 may need to change some logic since this patch was reverted.

[fedor.sergeev]

In D75404#1902311, @skan wrote:

In e-mail @reames wrote:

Er, I'm not sure this is correct. The concern is that boundary align requires the computation of the size of a fragment following the boundary align. Since the size may be influenced by the starting offset (e.g. align), the size of the boundary align must already be known. The circularity of the logic there seems likely to lead to issues, though I don't have a specific test case.

Though... I guess the way we use BA makes this "safe" as we know the instructions following must be either DataFragments or RelaxableFragments.

And unfortunately，I did find some test cases failed during runtime. I agree with reame, one difference between boundar align
and align fragment is that the size of boundary align depends on the following fragment while align not. Before finishLayout, we have to fix its size before we write boudary align and this has to be done in relaxBoundaryAlign. I feel sorry that D75203 may need to change some logic since this patch was reverted.

We also found this patch to cause problems with our downstream testing, though I do not have good reproducers as of right now.

llvm/include/llvm/MC/MCAssembler.h

private:
   /// if any offsets were adjusted.
   bool layoutSectionOnce(MCAsmLayout &Layout, MCSection &Sec);
 
   /// Perform relaxation on a single fragment - returns true if the fragment
   /// changes as a result of relaxation.
   bool relaxFragment(MCAsmLayout &Layout, MCFragment &F);
   bool relaxInstruction(MCAsmLayout &Layout, MCRelaxableFragment &IF);
   bool relaxLEB(MCAsmLayout &Layout, MCLEBFragment &IF);
-  bool relaxBoundaryAlign(MCAsmLayout &Layout, MCBoundaryAlignFragment &BF);
   bool relaxDwarfLineAddr(MCAsmLayout &Layout, MCDwarfLineAddrFragment &DF);
   bool relaxDwarfCallFrameFragment(MCAsmLayout &Layout,
                                    MCDwarfCallFrameFragment &DF);
   bool relaxCVInlineLineTable(MCAsmLayout &Layout,
                               MCCVInlineLineTableFragment &DF);
   bool relaxCVDefRange(MCAsmLayout &Layout, MCCVDefRangeFragment &DF);
 
   /// finishLayout - Finalize a layout, including fragment lowering.

llvm/include/llvm/MC/MCFragment.h

 class MCBoundaryAlignFragment : public MCFragment {
   /// The alignment requirement of the branch to be aligned.
   Align AlignBoundary;
   /// Flag to indicate whether the branch is fused.  Use in determining the
   /// region of fragments being aligned.
   bool Fused : 1;
   /// Flag to indicate whether NOPs should be emitted.
   bool EmitNops : 1;
-  /// The size of the fragment.  The size is lazily set during relaxation, and
-  /// is not meaningful before that.
-  uint64_t Size = 0;
 
 public:
   MCBoundaryAlignFragment(Align AlignBoundary = Align(1), bool Fused = false,
                           bool EmitNops = false, MCSection *Sec = nullptr)
       : MCFragment(FT_BoundaryAlign, false, Sec), AlignBoundary(AlignBoundary),
         Fused(Fused), EmitNops(EmitNops) {}
 
-  uint64_t getSize() const { return Size; }
-  void setSize(uint64_t Value) { Size = Value; }
-
   Align getAlignment() const { return AlignBoundary; }
   void setAlignment(Align Value) { AlignBoundary = Value; }
 
   bool isFused() const { return Fused; }
   void setFused(bool Value) { Fused = Value; }
 
   bool canEmitNops() const { return EmitNops; }
   void setEmitNops(bool Value) { EmitNops = Value; }
 
   static bool classof(const MCFragment *F) {
     return F->getKind() == MCFragment::FT_BoundaryAlign;
   }
 };
 } // end namespace llvm
 
 #endif // LLVM_MC_MCFRAGMENT_H

llvm/lib/MC/MCAssembler.cpp

bool MCAssembler::evaluateFixup(const MCAsmLayout &Layout,
   if (IsResolved && getBackend().shouldForceRelocation(*this, Fixup, Target)) {
     IsResolved = false;
     WasForced = true;
   }
 
   return IsResolved;
 }
 
+/// Check if the branch crosses the boundary.
+///
+/// \param StartAddr start address of the fused/unfused branch.
+/// \param Size size of the fused/unfused branch.
+/// \param BoundaryAlignment alignment requirement of the branch.
+/// \returns true if the branch cross the boundary.
+static bool mayCrossBoundary(uint64_t StartAddr, uint64_t Size,
+                             Align BoundaryAlignment) {
+  uint64_t EndAddr = StartAddr + Size;
+  return (StartAddr >> Log2(BoundaryAlignment)) !=
+         ((EndAddr - 1) >> Log2(BoundaryAlignment));
+}
+
+/// Check if the branch is against the boundary.
+///
+/// \param StartAddr start address of the fused/unfused branch.
+/// \param Size size of the fused/unfused branch.
+/// \param BoundaryAlignment alignment requirement of the branch.
+/// \returns true if the branch is against the boundary.
+static bool isAgainstBoundary(uint64_t StartAddr, uint64_t Size,
+                              Align BoundaryAlignment) {
+  uint64_t EndAddr = StartAddr + Size;
+  return (EndAddr & (BoundaryAlignment.value() - 1)) == 0;
+}
+
+/// Check if the branch needs padding.
+///
+/// \param StartAddr start address of the fused/unfused branch.
+/// \param Size size of the fused/unfused branch.
+/// \param BoundaryAlignment alignment requirement of the branch.
+/// \returns true if the branch needs padding.
+static bool needPadding(uint64_t StartAddr, uint64_t Size,
+                        Align BoundaryAlignment) {
+  return mayCrossBoundary(StartAddr, Size, BoundaryAlignment) ||
+         isAgainstBoundary(StartAddr, Size, BoundaryAlignment);
+}
+
 uint64_t MCAssembler::computeFragmentSize(const MCAsmLayout &Layout,
                                           const MCFragment &F) const {
   assert(getBackendPtr() && "Requires assembler backend");
   switch (F.getKind()) {
   case MCFragment::FT_Data:
     return cast<MCDataFragment>(F).getContents().size();
   case MCFragment::FT_Relaxable:
     return cast<MCRelaxableFragment>(F).getContents().size();
if (Size < 0) {
       return 0;
     }
     return Size;
   }
 
   case MCFragment::FT_LEB:
     return cast<MCLEBFragment>(F).getContents().size();
 
-  case MCFragment::FT_BoundaryAlign:
-    return cast<MCBoundaryAlignFragment>(F).getSize();
+  case MCFragment::FT_BoundaryAlign: {
+    const MCBoundaryAlignFragment &BF = cast<MCBoundaryAlignFragment>(F);
+    // MCBoundaryAlignFragment that doesn't emit NOP should have 0 size.
+    if (!BF.canEmitNops())
+      return 0;
+
+    uint64_t AlignedOffset = Layout.getFragmentOffset(&BF);
+    uint64_t AlignedSize = 0;
+    const MCFragment *F = BF.getNextNode();
+    // If the branch is unfused, it is emitted into one fragment, otherwise it
+    // is emitted into two fragments at most, the next
+    // MCBoundaryAlignFragment(if exists) also marks the end of the branch.
+    for (int I = 0, N = BF.isFused() ? 2 : 1;
+         I != N && !isa<MCBoundaryAlignFragment>(F); ++I, F = F->getNextNode())
+      AlignedSize += computeFragmentSize(Layout, *F);
+    Align BoundaryAlignment = BF.getAlignment();
+    return needPadding(AlignedOffset, AlignedSize, BoundaryAlignment)
+               ? offsetToAlignment(AlignedOffset, BoundaryAlignment)
+               : 0U;
+  }
 
   case MCFragment::FT_SymbolId:
     return 4;
 
   case MCFragment::FT_Align: {
     const MCAlignFragment &AF = cast<MCAlignFragment>(F);
     unsigned Offset = Layout.getFragmentOffset(&AF);
     unsigned Size = offsetToAlignment(Offset, Align(AF.getAlignment()));
bool MCAssembler::relaxLEB(MCAsmLayout &Layout, MCLEBFragment &LF) {
   // only increase an LEB fragment size here, not decrease it. See PR35809.
   if (LF.isSigned())
     encodeSLEB128(Value, OSE, OldSize);
   else
     encodeULEB128(Value, OSE, OldSize);
   return OldSize != LF.getContents().size();
 }
 
-/// Check if the branch crosses the boundary.
-///
-/// \param StartAddr start address of the fused/unfused branch.
-/// \param Size size of the fused/unfused branch.
-/// \param BoundaryAlignment alignment requirement of the branch.
-/// \returns true if the branch cross the boundary.
-static bool mayCrossBoundary(uint64_t StartAddr, uint64_t Size,
-                             Align BoundaryAlignment) {
-  uint64_t EndAddr = StartAddr + Size;
-  return (StartAddr >> Log2(BoundaryAlignment)) !=
-         ((EndAddr - 1) >> Log2(BoundaryAlignment));
-}
-
-/// Check if the branch is against the boundary.
-///
-/// \param StartAddr start address of the fused/unfused branch.
-/// \param Size size of the fused/unfused branch.
-/// \param BoundaryAlignment alignment requirement of the branch.
-/// \returns true if the branch is against the boundary.
-static bool isAgainstBoundary(uint64_t StartAddr, uint64_t Size,
-                              Align BoundaryAlignment) {
-  uint64_t EndAddr = StartAddr + Size;
-  return (EndAddr & (BoundaryAlignment.value() - 1)) == 0;
-}
-
-/// Check if the branch needs padding.
-///
-/// \param StartAddr start address of the fused/unfused branch.
-/// \param Size size of the fused/unfused branch.
-/// \param BoundaryAlignment alignment requirement of the branch.
-/// \returns true if the branch needs padding.
-static bool needPadding(uint64_t StartAddr, uint64_t Size,
-                        Align BoundaryAlignment) {
-  return mayCrossBoundary(StartAddr, Size, BoundaryAlignment) ||
-         isAgainstBoundary(StartAddr, Size, BoundaryAlignment);
-}
-
-bool MCAssembler::relaxBoundaryAlign(MCAsmLayout &Layout,
-                                     MCBoundaryAlignFragment &BF) {
-  // The MCBoundaryAlignFragment that doesn't emit NOP should not be relaxed.
-  if (!BF.canEmitNops())
-    return false;
-
-  uint64_t AlignedOffset = Layout.getFragmentOffset(BF.getNextNode());
-  uint64_t AlignedSize = 0;
-  const MCFragment *F = BF.getNextNode();
-  // If the branch is unfused, it is emitted into one fragment, otherwise it is
-  // emitted into two fragments at most, the next MCBoundaryAlignFragment(if
-  // exists) also marks the end of the branch.
-  for (auto i = 0, N = BF.isFused() ? 2 : 1;
-       i != N && !isa<MCBoundaryAlignFragment>(F); ++i, F = F->getNextNode()) {
-    AlignedSize += computeFragmentSize(Layout, *F);
-  }
-  uint64_t OldSize = BF.getSize();
-  AlignedOffset -= OldSize;
-  Align BoundaryAlignment = BF.getAlignment();
-  uint64_t NewSize = needPadding(AlignedOffset, AlignedSize, BoundaryAlignment)
-                         ? offsetToAlignment(AlignedOffset, BoundaryAlignment)
-                         : 0U;
-  if (NewSize == OldSize)
-    return false;
-  BF.setSize(NewSize);
-  Layout.invalidateFragmentsFrom(&BF);
-  return true;
-}
-
 bool MCAssembler::relaxDwarfLineAddr(MCAsmLayout &Layout,
                                      MCDwarfLineAddrFragment &DF) {
   MCContext &Context = Layout.getAssembler().getContext();
   uint64_t OldSize = DF.getContents().size();
   int64_t AddrDelta;
   bool Abs = DF.getAddrDelta().evaluateKnownAbsolute(AddrDelta, Layout);
   assert(Abs && "We created a line delta with an invalid expression");
   (void)Abs;
case MCFragment::FT_Relaxable:
     return relaxInstruction(Layout, cast<MCRelaxableFragment>(F));
   case MCFragment::FT_Dwarf:
     return relaxDwarfLineAddr(Layout, cast<MCDwarfLineAddrFragment>(F));
   case MCFragment::FT_DwarfFrame:
     return relaxDwarfCallFrameFragment(Layout,
                                        cast<MCDwarfCallFrameFragment>(F));
   case MCFragment::FT_LEB:
     return relaxLEB(Layout, cast<MCLEBFragment>(F));
-  case MCFragment::FT_BoundaryAlign:
-    return relaxBoundaryAlign(Layout, cast<MCBoundaryAlignFragment>(F));
   case MCFragment::FT_CVInlineLines:
     return relaxCVInlineLineTable(Layout, cast<MCCVInlineLineTableFragment>(F));
   case MCFragment::FT_CVDefRange:
     return relaxCVDefRange(Layout, cast<MCCVDefRangeFragment>(F));
   }
 }
 
 bool MCAssembler::layoutSectionOnce(MCAsmLayout &Layout, MCSection &Sec) {

llvm/lib/MC/MCFragment.cpp

case MCFragment::FT_BoundaryAlign: {
     const auto *BF = cast<MCBoundaryAlignFragment>(this);
     if (BF->canEmitNops())
       OS << " (can emit nops to align";
     if (BF->isFused())
       OS << " fused branch)";
     else
       OS << " unfused branch)";
     OS << "\n       ";
-    OS << " BoundarySize:" << BF->getAlignment().value()
-       << " Size:" << BF->getSize();
+    OS << " BoundarySize:" << BF->getAlignment().value();
     break;
   }
   case MCFragment::FT_SymbolId: {
     const auto *F = cast<MCSymbolIdFragment>(this);
     OS << "\n       ";
     OS << " Sym:" << F->getSymbol();
     break;
   }

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

 }
 
 /// Insert a MCBoundaryAlignFragment to mark the end of the branch to be aligned
 /// if necessary.
 void X86AsmBackend::alignBranchesEnd(MCObjectStreamer &OS, const MCInst &Inst) {
   if (!needAlign(OS))
     return;
   // If the branch is emitted into a MCRelaxableFragment, we can determine the
-  // size of the branch easily in MCAssembler::relaxBoundaryAlign. When the
+  // size of the branch easily in during the process of layout. When the
   // branch is fused, the fused branch(macro fusion pair) must be emitted into
   // two fragments. Or when the branch is unfused, the branch must be emitted
   // into one fragment. The MCRelaxableFragment naturally marks the end of the
   // fused or unfused branch.
   // Otherwise, we need to insert a MCBoundaryAlignFragment to mark the end of
   // the branch. This MCBoundaryAlignFragment may be reused to emit NOP to align
   // other branch.
   if (needAlignInst(Inst) && !isa<MCRelaxableFragment>(OS.getCurrentFragment()))