[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

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

10

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

[skan]

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

106

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

10

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

10

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

10

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-basic.s

test_indirect:
   .endr
   jmpq *(%rax)
 
   .p2align 4
   .type   bar,@function
 bar:
   retq
 
-  # CHECK: <test_pad_via_relax>:
-  # CHECK: 200: testq
-  # CHECK: 203: jne
-  # CHECK: 209: int3
-  # note 6 byte jne which could be a 2 byte jne, but is instead
-  # expanded for padding purposes
-  # CHECK-NOT: nop
-  # CHECK: 220: callq
-  .global test_pad_via_relax
-  .p2align  5
-test_pad_via_relax:
-  testq %rax, %rax
-  jnz bar
-  .rept 23
-  int3
-  .endr
-  callq bar
-
   # This case looks really tempting to pad, but doing so for the call causes
   # the jmp to be misaligned.
   # CHECK: <test_pad_via_relax_neg1>:
-  # CHECK: 240: int3
-  # CHECK: 25a: testq
-  # CHECK: 25d: jne
-  # CHECK: 25f: nop
-  # CHECK: 260: callq
+  # CHECK: 200: int3
+  # CHECK: 21a: testq
+  # CHECK: 21d: jne
+  # CHECK: 21f: nop
+  # CHECK: 220: callq
   .global test_pad_via_relax_neg1
   .p2align  5
 test_pad_via_relax_neg1:
   .rept 26
   int3
   .endr
   testq %rax, %rax
   jnz bar
   callq bar
 
   # Same as previous, but without fusion
   # CHECK: <test_pad_via_relax_neg2>:
-  # CHECK: 280: int3
-  # CHECK: 29d: jmp
-  # CHECK: 29f: nop
-  # CHECK: 2a0: callq
+  # CHECK: 240: int3
+  # CHECK: 25d: jmp
+  # CHECK: 25f: nop
+  # CHECK: 260: callq
   .global test_pad_via_relax_neg2
   .p2align  5
 test_pad_via_relax_neg2:
   .rept 29
   int3
   .endr
   jmp bar2
   callq bar2
 
-bar2: 
+bar2:
 
   .section "unknown"
   .p2align 4
   .type   baz,@function
 baz:
   retq

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