[llvm] 2ac19fe - [X86] Not track size of the boudaryalign fragment during the layout
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 2 09:09:31 PST 2020
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.
At the minimum, this probably deserves a careful comment.
Philip
On 3/1/20 5:33 PM, Shengchen Kan via llvm-commits wrote:
> Author: Shengchen Kan
> Date: 2020-03-02T09:32:30+08:00
> New Revision: 2ac19feb1571960b8e1479a451b45ab56da7034e
>
> URL: https://github.com/llvm/llvm-project/commit/2ac19feb1571960b8e1479a451b45ab56da7034e
> DIFF: https://github.com/llvm/llvm-project/commit/2ac19feb1571960b8e1479a451b45ab56da7034e.diff
>
> LOG: [X86] Not track size of the boudaryalign fragment during the layout
>
> 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.
>
> Reviewers: annita.zhang, reames, MaskRay, craig.topper, LuoYuanke, jyknight
>
> Reviewed By: MaskRay
>
> Subscribers: hiraditya, dexonsmith, llvm-commits
>
> Tags: #llvm
>
> Differential Revision: https://reviews.llvm.org/D75404
>
> Added:
>
>
> Modified:
> llvm/include/llvm/MC/MCAssembler.h
> llvm/include/llvm/MC/MCFragment.h
> llvm/lib/MC/MCAssembler.cpp
> llvm/lib/MC/MCFragment.cpp
> llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
>
> Removed:
>
>
>
> ################################################################################
> diff --git a/llvm/include/llvm/MC/MCAssembler.h b/llvm/include/llvm/MC/MCAssembler.h
> index b57439f02ca5..caa392a41b2b 100644
> --- a/llvm/include/llvm/MC/MCAssembler.h
> +++ b/llvm/include/llvm/MC/MCAssembler.h
> @@ -195,7 +195,6 @@ class MCAssembler {
> 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);
>
> diff --git a/llvm/include/llvm/MC/MCFragment.h b/llvm/include/llvm/MC/MCFragment.h
> index e052098611a9..610e924f7846 100644
> --- a/llvm/include/llvm/MC/MCFragment.h
> +++ b/llvm/include/llvm/MC/MCFragment.h
> @@ -528,9 +528,6 @@ class MCBoundaryAlignFragment : public MCFragment {
> 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,
> @@ -538,9 +535,6 @@ class MCBoundaryAlignFragment : public MCFragment {
> : 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; }
>
>
> diff --git a/llvm/lib/MC/MCAssembler.cpp b/llvm/lib/MC/MCAssembler.cpp
> index bcfda261d3ec..8582d5143aa8 100644
> --- a/llvm/lib/MC/MCAssembler.cpp
> +++ b/llvm/lib/MC/MCAssembler.cpp
> @@ -285,6 +285,43 @@ bool MCAssembler::evaluateFixup(const MCAsmLayout &Layout,
> 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");
> @@ -314,8 +351,26 @@ uint64_t MCAssembler::computeFragmentSize(const MCAsmLayout &Layout,
> 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;
> @@ -957,72 +1012,6 @@ bool MCAssembler::relaxLEB(MCAsmLayout &Layout, MCLEBFragment &LF) {
> 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();
> @@ -1123,8 +1112,6 @@ bool MCAssembler::relaxFragment(MCAsmLayout &Layout, MCFragment &F) {
> 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:
>
> diff --git a/llvm/lib/MC/MCFragment.cpp b/llvm/lib/MC/MCFragment.cpp
> index a96b8e86aed3..42ba3b40c51f 100644
> --- a/llvm/lib/MC/MCFragment.cpp
> +++ b/llvm/lib/MC/MCFragment.cpp
> @@ -431,8 +431,7 @@ LLVM_DUMP_METHOD void MCFragment::dump() const {
> else
> OS << " unfused branch)";
> OS << "\n ";
> - OS << " BoundarySize:" << BF->getAlignment().value()
> - << " Size:" << BF->getSize();
> + OS << " BoundarySize:" << BF->getAlignment().value();
> break;
> }
> case MCFragment::FT_SymbolId: {
>
> diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp b/llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
> index 8ec3f4241c3e..067748fdb1f8 100644
> --- a/llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
> +++ b/llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
> @@ -442,7 +442,7 @@ 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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list