[X86][MC] Disable Prefix padding after hardcode/prefix

[skan]

Rebase

[craig.topper]

llvm/test/MC/X86/align-via-padding-corner.s

6

There is an 'a' before and after the line break so this says "on a a instruction"

[skan]

Address review comments

[craig.topper]

LGTM

llvm/include/llvm/MC/MCFragment.h

 
 /// A relaxable fragment holds on to its MCInst, since it may need to be
 /// relaxed during the assembler layout and relaxation stage.
 ///
 class MCRelaxableFragment : public MCEncodedFragmentWithFixups<8, 1> {
 
   /// The instruction this is a fragment for.
   MCInst Inst;
+  /// Can we auto pad the instruction?
+  bool AllowAutoPadding = false;
 
 public:
   MCRelaxableFragment(const MCInst &Inst, const MCSubtargetInfo &STI,
                       MCSection *Sec = nullptr)
       : MCEncodedFragmentWithFixups(FT_Relaxable, true, Sec),
         Inst(Inst) { this->STI = &STI; }
 
   const MCInst &getInst() const { return Inst; }
   void setInst(const MCInst &Value) { Inst = Value; }
 
+  bool getAllowAutoPadding() const { return AllowAutoPadding; }
+  void setAllowAutoPadding(bool V) { AllowAutoPadding = V; }
+
   static bool classof(const MCFragment *F) {
     return F->getKind() == MCFragment::FT_Relaxable;
   }
 };
 
 class MCAlignFragment : public MCFragment {
   /// The alignment to ensure, in bytes.
   unsigned Alignment;

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

class X86AsmBackend : public MCAsmBackend {
   Align AlignBoundary;
 
   uint8_t determinePaddingPrefix(const MCInst &Inst) const;
 
   bool isMacroFused(const MCInst &Cmp, const MCInst &Jcc) const;
 
   bool needAlign(MCObjectStreamer &OS) const;
   bool needAlignInst(const MCInst &Inst) const;
+  bool allowAutoPaddingForInst(const MCInst &Inst, MCObjectStreamer &OS) const;
   MCInst PrevInst;
   MCBoundaryAlignFragment *PendingBoundaryAlign = nullptr;
   std::pair<MCFragment *, size_t> PrevInstPosition;
+  bool AllowAutoPaddingForInst;
 
 public:
   X86AsmBackend(const Target &T, const MCSubtargetInfo &STI)
       : MCAsmBackend(support::little), STI(STI),
         MCII(T.createMCInstrInfo()) {
     if (X86AlignBranchWithin32BBoundaries) {
       // At the moment, this defaults to aligning fused branches, unconditional
       // jumps, and (unfused) conditional jumps with nops.  Both the
case MCFragment::FT_Data:
     return cast<MCDataFragment>(*F).getContents().size();
   case MCFragment::FT_Relaxable:
     return cast<MCRelaxableFragment>(*F).getContents().size();
   case MCFragment::FT_CompactEncodedInst:
     return cast<MCCompactEncodedInstFragment>(*F).getContents().size();
   }
 }
 
-/// Check if the instruction operand needs to be aligned. Padding is disabled
-/// before intruction which may be rewritten by linker(e.g. TLSCALL).
+/// Check if the instruction operand needs to be aligned.
 bool X86AsmBackend::needAlignInst(const MCInst &Inst) const {
-  // Linker may rewrite the instruction with variant symbol operand.
-  if (hasVariantSymbol(Inst))
-    return false;
-
   const MCInstrDesc &InstDesc = MCII->get(Inst.getOpcode());
   return (InstDesc.isConditionalBranch() &&
           (AlignBranchType & X86::AlignBranchJcc)) ||
          (InstDesc.isUnconditionalBranch() &&
           (AlignBranchType & X86::AlignBranchJmp)) ||
          (InstDesc.isCall() &&
           (AlignBranchType & X86::AlignBranchCall)) ||
          (InstDesc.isReturn() &&
           (AlignBranchType & X86::AlignBranchRet)) ||
          (InstDesc.isIndirectBranch() &&
           (AlignBranchType & X86::AlignBranchIndirect));
 }
 
-/// Insert BoundaryAlignFragment before instructions to align branches.
-void X86AsmBackend::emitInstructionBegin(MCObjectStreamer &OS,
-                                       const MCInst &Inst) {
-  if (!needAlign(OS))
-    return;
+/// Return true if we can insert NOP or prefixes automatically before the
+/// the instruction to be emitted.
+bool X86AsmBackend::allowAutoPaddingForInst(const MCInst &Inst,
+                                            MCObjectStreamer &OS) const {
+  if (hasVariantSymbol(Inst))
+    // Linker may rewrite the instruction with variant symbol operand(e.g.
+    // TLSCALL).
+    return false;
 
   if (hasInterruptDelaySlot(PrevInst))
     // If this instruction follows an interrupt enabling instruction with a one
     // instruction delay, inserting a nop would change behavior.
-    return;
+    return false;
 
   if (isPrefix(PrevInst, *MCII))
-    // If this instruction follows a prefix, inserting a nop would change
+    // If this instruction follows a prefix, inserting a nop/prefix would change
     // semantic.
-    return;
+    return false;
+
+  if (isPrefix(Inst, *MCII))
+    // If this instruction is a prefix, inserting a prefix would change
+    // semantic.
+    return false;
 
   if (isRightAfterData(OS.getCurrentFragment(), PrevInstPosition))
     // If this instruction follows any data, there is no clear
-    // instruction boundary, inserting a nop would change semantic.
+    // instruction boundary, inserting a nop/prefix would change semantic.
+    return false;
+
+  return true;
+}
+
+/// Insert BoundaryAlignFragment before instructions to align branches.
+void X86AsmBackend::emitInstructionBegin(MCObjectStreamer &OS,
+                                         const MCInst &Inst) {
+  AllowAutoPaddingForInst = allowAutoPaddingForInst(Inst, OS);
+
+  if (!needAlign(OS))
     return;
 
   if (!isMacroFused(PrevInst, Inst))
     // Macro fusion doesn't happen indeed, clear the pending.
     PendingBoundaryAlign = nullptr;
 
+  if (!AllowAutoPaddingForInst)
+    return;
+
   if (PendingBoundaryAlign &&
       OS.getCurrentFragment()->getPrevNode() == PendingBoundaryAlign) {
     // Macro fusion actually happens and there is no other fragment inserted
     // after the previous instruction.
     //
     // Do nothing here since we already inserted a BoudaryAlign fragment when
     // we met the first instruction in the fused pair and we'll tie them
     // together in emitInstructionEnd.
if (needAlignInst(Inst) || ((AlignBranchType & X86::AlignBranchFused) &&
     // insert a BoundaryAlign fragment.
     OS.insert(PendingBoundaryAlign =
                   new MCBoundaryAlignFragment(AlignBoundary));
   }
 }
 
 /// Set the last fragment to be aligned for the BoundaryAlignFragment.
 void X86AsmBackend::emitInstructionEnd(MCObjectStreamer &OS, const MCInst &Inst) {
-  if (!needAlign(OS))
-    return;
-
   PrevInst = Inst;
   MCFragment *CF = OS.getCurrentFragment();
   PrevInstPosition = std::make_pair(CF, getSizeForInstFragment(CF));
+  if (auto *F = dyn_cast_or_null<MCRelaxableFragment>(CF))
+    F->setAllowAutoPadding(AllowAutoPaddingForInst);
+
+  if (!needAlign(OS))
+    return;
 
   if (!needAlignInst(Inst) || !PendingBoundaryAlign)
     return;
 
   // Tie the aligned instructions into a a pending BoundaryAlign.
   PendingBoundaryAlign->setLastFragment(CF);
   PendingBoundaryAlign = nullptr;
 

 /// general available form.
 static bool isFullyRelaxed(const MCRelaxableFragment &RF) {
   auto &Inst = RF.getInst();
   auto &STI = *RF.getSubtargetInfo();
   bool Is16BitMode = STI.getFeatureBits()[X86::Mode16Bit];
   return getRelaxedOpcode(Inst, Is16BitMode) == Inst.getOpcode();
 }
 
-
-static bool shouldAddPrefix(const MCInst &Inst, const MCInstrInfo &MCII) {
-  // Linker may rewrite the instruction with variant symbol operand.
-  return !hasVariantSymbol(Inst);
-}
-
 static unsigned getRemainingPrefixSize(const MCInst &Inst,
                                        const MCSubtargetInfo &STI,
                                        MCCodeEmitter &Emitter) {
   SmallString<256> Code;
   raw_svector_ostream VecOS(Code);
   Emitter.emitPrefix(Inst, VecOS, STI);
   assert(Code.size() < 15 && "The number of prefixes must be less than 15.");
 
   // TODO: It turns out we need a decent amount of plumbing for the target
   // specific bits to determine number of prefixes its safe to add.  Various
   // targets (older chips mostly, but also Atom family) encounter decoder
   // stalls with too many prefixes.  For testing purposes, we set the value
   // externally for the moment.
   unsigned ExistingPrefixSize = Code.size();
   unsigned TargetPrefixMax = X86PadMaxPrefixSize;
   if (TargetPrefixMax <= ExistingPrefixSize)
     return 0;
   return TargetPrefixMax - ExistingPrefixSize;
 }
 
 bool X86AsmBackend::padInstructionViaPrefix(MCRelaxableFragment &RF,
                                             MCCodeEmitter &Emitter,
                                             unsigned &RemainingSize) const {
-  if (!shouldAddPrefix(RF.getInst(), *MCII))
+  if (!RF.getAllowAutoPadding())
     return false;
   // If the instruction isn't fully relaxed, shifting it around might require a
   // larger value for one of the fixups then can be encoded.  The outer loop
   // will also catch this before moving to the next instruction, but we need to
   // prevent padding this single instruction as well.
   if (!isFullyRelaxed(RF))
     return false;
 

llvm/test/MC/X86/align-via-padding-corner.s

+  # RUN: llvm-mc -mcpu=skylake -filetype=obj -triple x86_64-pc-linux-gnu %s -x86-pad-max-prefix-size=5 | llvm-objdump -d - | FileCheck %s
+
+
+  # The first test check the correctness cornercase - can't add prefixes on a
+  # instruction following by a prefix.
+  .globl labeled_prefix_test
+labeled_prefix_test:
+# CHECK:       0: 2e 2e 2e 2e 2e e9 06 00 00 00    jmp
+# CHECK:       a: 3e e9 00 00 00 00                jmp
+  jmp bar
+  DS
+  jmp bar
+  .p2align 4
+bar:
+  ret
+
+  # The second test is similar to the second test - can't add prefixes on a
+  # instruction following by hardcode.
+  .p2align 5
+  .globl labeled_hardcode_test
+labeled_hardcode_test:
+# CHECK:      20: 2e 2e 2e 2e 2e e9 06 00 00 00    jmp
+# CHECK:      2a: 3e e9 00 00 00 00                jmp
+  jmp baz
+  .byte 0x3e
+  jmp baz
+  .p2align 4
+baz:
+  ret