MaskRay added a comment. Haven't looked into too many details yet but made some suggestions anyway...
================ Comment at: llvm/include/llvm/MC/MCFragment.h:634 + + uint64_t getBoundarySize() const { + return AlignBoundarySize; ---------------- Store AlignBoundarySize as a shift value, then `needPadding` doesn't even need to call Log2_64(). ================ Comment at: llvm/lib/MC/MCFragment.cpp:426 + case MCFragment::FT_MachineDependent: { + const MCMachineDependentFragment *MF = + cast<MCMachineDependentFragment>(this); ---------------- `const auto *`. The type is obvious according to the right hand side. ================ Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:104 + else if (BranchType == "indirect") + addKind(AlignBranchIndirect); + } ---------------- An unknown value is just ignored, e.g. `--x86-align-branch=unknown`. I think there should be an error, but I haven't looked into the patch detail to confidently suggest how we should surface this error. ================ Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:185 + AlignMaxPrefixSize = 5; + } else { + assert((X86AlignBranchBoundary == 0 || (X86AlignBranchBoundary >= 32 && ---------------- > } else { Should --x86-branches-within-32B-boundaries overwrite --x86-align-branch-boundary and --x86-align-branch and --x86-align-branch-prefix-size? My feeling is that it just provides a default value if either of the three options is not specified. If you are going to remove `addKind` calls here, you can delete this member function. ================ Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:196 + } + if(AlignBoundarySize != 0) { + AlignBoundarySize = Align(AlignBoundarySize).value(); ---------------- space after if No curly braces around simple statements. ================ Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:660 + for (auto *F = CF; F && F != LastBranch; F = F->getPrevNode()) { + bool IsMF = F->getKind() == MCFragment::FT_MachineDependent; + // If there is a fragment that neither has instructions nor is ---------------- `isa<MCMachineDependentFragment>(F)` ================ Comment at: llvm/lib/Target/X86/X86InstrInfo.td:1020 def X86_COND_B : PatLeaf<(i8 2)>; // alt. COND_C -def X86_COND_AE : PatLeaf<(i8 3)>; // alt. COND_NC +def X86_COND_AE : PatLeaf<(i8 3)>; // alt. COND_NC,COND_NB def X86_COND_E : PatLeaf<(i8 4)>; // alt. COND_Z ---------------- Unintentional change not part of this patch? ================ Comment at: llvm/test/MC/X86/align-branch-32-1a.s:1 +# Check the macro-fusion table +# RUN: llvm-mc -filetype=obj -triple i386-unknown-unknown --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc+jmp --x86-align-branch-prefix-size=5 %s | llvm-objdump -d - | FileCheck %s ---------------- Did an older version include 32-1b.s or 32-1c.s? Now they are missing. ================ Comment at: llvm/test/MC/X86/align-branch-64-1b.s:1 +# Check only fused conditional jumps and conditional jumps are aligned with option --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc --x86-align-branch-prefix-size=5 +# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc --x86-align-branch-prefix-size=5 %s | llvm-objdump -d - | FileCheck %s ---------------- Create test/MC/X86/Inputs/align-branch-64-1.s and reference it from 1[a-d].s via %S/Inputs/align-branch-64-1.s CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70157/new/ https://reviews.llvm.org/D70157 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits