[X86][MC] Fix the bug for prefix padding support

[skan]

Summary

There is a tiny logic error of D75300, making branch is not
correctly aligned with option -x86-pad-max-prefix-size

[skan]

Delete the unimplemented check in test file (we haven't supported relaxing
the branches to be aligned to reduce the bytes of nops)

[reames]

It's not clear what bug you think you're fixing. Please provide a description of the bad behaviour.

llvm/test/MC/X86/align-branch-64-basic.s

106 ↗

(On Diff #250964)

Please restore this test and show any diff. This test is not an unimplemented case, it's testing the jne being relaxed which is happening.

llvm/test/MC/X86/align-branch-64-pad-max-prefix.s

11

This case should already be handled by skipping instructions past the branch align.

[skan]

llvm/test/MC/X86/align-branch-64-basic.s

106 ↗

(On Diff #250964)

Why do you think we can relax this jne to reduce nops? This jne is after boundaryalign.

llvm/test/MC/X86/align-branch-64-pad-max-prefix.s

11

No, it's not handled. Your can run this test case without this patch to prove.

[LuoYuanke]

llvm/test/MC/X86/align-branch-64-pad-max-prefix.s

11

Do you mean when jmp size is expanded, it cross the align boundary? That seems a problem. The jmp size should be frozen after nop padding.

[skan]

llvm/test/MC/X86/align-branch-64-pad-max-prefix.s

11

Yes, the jump's size is expanded and leans against the boundary. This patch is trying to fix this bug.

[skan]

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

973–977

The existing code in finishLayout intends to not pad instructions following boundary align by checking whether F is MCBoundaryAlignFragment. But this early continue prevents the control flow from reaching that check. This patch is pretty small (delete 3 lines) and is a bug fix, could reviewers take a close look at it?

[reames]

LGTM, thank you for the explanation from Tuesday, sorry for not noticing that sooner.

[MaskRay]

llvm/test/MC/X86/align-branch-64-pad-max-prefix.s

2

I am late, but the indentation is really not needed.

-pc-linux-gnu is really not needed.

15

Add -CHECK: if applicable.

[skan]

llvm/test/MC/X86/align-branch-64-pad-max-prefix.s

2

Almost all test cases align-branch* use the indentation , -pc-linux-gnu , we can delete them all at once later if necessary.

15

sorry, didn't get your point.

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

for (MCSection::iterator I = Sec.begin(), IE = Sec.end(); I != IE; ++I) {
         Relaxable.clear();
         continue;
       }
 
 #ifndef NDEBUG
       const uint64_t OrigOffset = Layout.getFragmentOffset(&F);
 #endif
       const uint64_t OrigSize = Asm.computeFragmentSize(Layout, F);
-      if (OrigSize == 0 || Relaxable.empty()) {
-        Relaxable.clear();
-        continue;
-      }
 
       // To keep the effects local, prefer to relax instructions closest to
       // the align directive.  This is purely about human understandability
       // of the resulting code.  If we later find a reason to expand
       // particular instructions over others, we can adjust.
       MCFragment *FirstChangedFragment = nullptr;
       unsigned RemainingSize = OrigSize;
       while (!Relaxable.empty() && RemainingSize != 0) {
         auto &RF = *Relaxable.pop_back_val();

llvm/test/MC/X86/align-branch-64-pad-max-prefix.s

+  # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu --x86-align-branch-boundary=32 --x86-align-branch=jmp -x86-pad-max-prefix-size=5 %s | llvm-objdump -d --no-show-raw-insn - | FileCheck %s
+  # Check instructions can be aligned correctly along with option -x86-pad-max-prefix-size=5
+
+  .text
+  .p2align 5
+  .rept 24
+  int3
+  .endr
+  # We should not increase the length of this jmp to reduce the bytes of
+  # following nops, doing so would make the jmp misaligned.
+# CHECK:      18:          jmp
+  jmp bar
+# CHECK:      1d:          nop
+# CHECK:      1e:          nop
+# CHECK:      1f:          nop
+# CHECK:      20:          int3
+  .p2align 5
+  int3