[X86] support .nops directive

[jcai19]

Summary

Add support of .nops on X86. This addresses llvm.org/PR45788.

[craig.topper]

Would it be possible to emit a new Fragment type and leverage the existing NOP emission code in X86AsmBackend.cpp. The code here appears to be copied from X86MCInstLower. Which would mean we would now have 3 places that do almost the same thing.

[jcai19]

In D82826#2121595, @craig.topper wrote:

Would it be possible to emit a new Fragment type and leverage the existing NOP emission code in X86AsmBackend.cpp. The code here appears to be copied from X86MCInstLower. Which would mean we would now have 3 places that do almost the same thing.

Thanks for the suggestion. Did not know there was a second copy already. Will try to reuse the code from X86AsmBackend.cpp.

[MaskRay]

I asked what the largest operand .nops supports https://sourceware.org/bugzilla/show_bug.cgi?id=25789 but did not get a clear answer. Maybe some reviewer can help me ask for a clarification...

llvm/test/MC/X86/x86-directive-nops.s

1

#

7

# X32

[nickdesaulniers]

If NopSize is basically equivalent for NumBytes except for 2 cases, then I'd do:

if (!NopSize) {
  print_some_error();
  return;
}
unsigned NopSize = NumBytes;
switch (NumBytes) {
...
default:
  NopSize = 10;
  ...
}

So that you don't have to assign NopSize = NumBytes for each other case.

[jcai19]

@craig.topper I tried to add a new Fragment type and use X86AsmBackend::writeNopData to insert NOP instructions. It however changed the outcome of the test cases I used, as X86AsmBackend::writeNopData only generatssingle-byte nop instructions if X86::FeatureNOPL feature bit is not true, while the maxLongNopLength in the current implementation (from X86MCInstLower) allows long nop instructions up to 10 bytes on a 64-bit processor. Which one is preferred? Also, which target triple should I use to test multiple-byte nop instructions? Thanks.

[jcai19]

@MaskRay @nickdesaulniers Thanks for the comments. Will refactor the code once we decide which way to go with the implementation.

[craig.topper]

In D82826#2124581, @jcai19 wrote:

@craig.topper I tried to add a new Fragment type and use X86AsmBackend::writeNopData to insert NOP instructions. It however changed the outcome of the test cases I used, as X86AsmBackend::writeNopData only generatssingle-byte nop instructions if X86::FeatureNOPL feature bit is not true, while the maxLongNopLength in the current implementation (from X86MCInstLower) allows long nop instructions up to 10 bytes on a 64-bit processor. Which one is preferred? Also, which target triple should I use to test multiple-byte nop instructions? Thanks.

That seems like a bug in writeNopData. All real 64-bit CPUs should have FeatureNOPL. But I think llvm-mc and llc default to "generic" as a CPU which isn't a real CPU and doesn't have the feature. clang never uses the "generic". The default 64-bit CPU for clang is "x86-64" which does have the feature. I'll see what happens if we check for 64-bit mode explicitly in writeNopData and post a patch tonight or tomorrow.

[jcai19]

Thanks! Checking if the CPU is 64-bit along FeatureNOPL seemed to work. Will verify more. Also I'd like to point out the difference of 32-bit CPUs.

[craig.topper]

writeNopsData should always use multibyte nops in 64-bit mode now.

[craig.topper]

is the 32-bit different the 2 byte case?

[jcai19]

In D82826#2126190, @craig.topper wrote:

is the 32-bit different the 2 byte case?

Yes.

[MaskRay]

I have a concern that we haven't figured out the largest operand .nops supports. We should clarify this (https://sourceware.org/bugzilla/show_bug.cgi?id=25789 )

[jcai19]

(1) This is a temporary implementation as it breaks the following tests:

LLVM :: MC/COFF/align-nops.s
LLVM :: MC/MachO/x86_32-optimal_nop.s
LLVM :: MC/X86/AlignedBundling/misaligned-bundle-group.s
LLVM :: MC/X86/AlignedBundling/misaligned-bundle.s
LLVM :: MC/X86/align-branch-bundle.s
LLVM :: MC/X86/align-branch-pad-max-prefix.s
LLVM :: MC/X86/x86_long_nop.s
LLVM :: MC/X86/x86_nop.s

The breakage was caused by checking X86::Mode32Bit and X86::Mode64Bit in the newly introduced X86AsmBackend::getMaximumNopSize() function. All the above tests would pass if I removed the two checks, but .nops calls would then emit byte-long nop instructions regardless of its second argument.

(2) Will address error messages in the next iteration.

[nickdesaulniers]

I'm having trouble posting comments inline; it seems fabricator won't allow me to "save" comments.

Is it too painful to make the member of MCNopsFragment uint64_t? a la MCStreamer::emitFill

[craig.topper]

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

1073 ↗

(On Diff #274963)

Mode32Bit is only relevant if FeatureNOPL is false. So I think it should be

if (!STI.getFeatureBits()[X86::FeatureNOPL] &&
    !STI.getFeatureBits()[X86::Mode64Bit])
  return STI.getFeatureBits()[X86::Mode32Bit] ? 2 : 1;

[jcai19]

In D82826#2126955, @nickdesaulniers wrote:

I'm having trouble posting comments inline; it seems fabricator won't allow me to "save" comments.

Is it too painful to make the member of MCNopsFragment uint64_t? a la MCStreamer::emitFill

I suppose it's easy to do but I'll have to explicitly cast them to signed integers to check if they are smaller than 0. That is probably more error-prone IMO. WDYT?

[jcai19]

Refactor X86AsmBackend::getMaximumNopSize based on @craig.topper's comment.

[jcai19]

Issuing multiple-byte NOP for 32-bit mode broke many tests and probably worth a separate patch itself. So this patch will keep the behavior on 32-bit mode unchange for now. This built and passed all the tests.

[reames]

llvm/include/llvm/MC/MCFragment.h

358 ↗

(On Diff #275853)

Can you call this MaxNopLength or something?

llvm/lib/MC/MCAssembler.cpp

625 ↗

(On Diff #275853)

When parsing asm, you reject negative lengths. Should these simply be asserts?

633 ↗

(On Diff #275853)

Does this behaviour match existing gnu? I'd have expected the result of specifying a "too large" maximum size to simply clamp to the target's maximum.

This is important as if the result is semantic, then the difference between "largest encodeable" and "largest profitable" becomes a thing the rest of the code has to care about. 15 byte nops are almost always *legal* they're just not *fast*.

644 ↗

(On Diff #275853)

This loop is duplicated from within emitNops. Can you pass in a MaxNopLength parameter instead?

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

1072 ↗

(On Diff #275853)

Rename this function to getMaximumProfitableNop()

There's a difference between legality and profit here. As commented earlier, if that matters you'll have a harder task implementation wise.

llvm/test/MC/X86/align-branch-bundle.s

9 ↗

(On Diff #275853)

Having a test delta in a file without .nops is highly suspicious.

I'd suggest splitting your patch into a trivial version which emits single byte nops, and an change which adds the multiple byte support. That would allow us to separate the directive mechanics from the interesting profit bits.

[jcai19]

llvm/lib/MC/MCAssembler.cpp

633 ↗

(On Diff #275853)

Does this behaviour match existing gnu?

Appears so.

$ cat foo.s
.nops 16, 15

$ gcc -c foo.s
foo.s: Assembler messages:
foo.s:1: Error: invalid single nop size: 15 (expect within [0, 11])

With the patch applied,
$ llvm-mc -filetype=obj -triple=x86_64 foo.s
foo.s:1:1: error: illegal NOP size 15. (expected within [0, 10])
.nops 16, 15
^

llvm/test/MC/X86/align-branch-bundle.s

9 ↗

(On Diff #275853)

How about we also print out instruction bytes here. If 64-bit processors can generate a two-byte long nop instruction here, shouldn't we emit that instead of two single-byte nop? Thanks.

[jcai19]

llvm/lib/MC/MCAssembler.cpp

644 ↗

(On Diff #275853)

There isn't any loop in emitNops. Do you by any chance refer to the loop in writeNopData? In that case, they are not duplicate as this loop will break total bytes into nop instructions no longer than specified by the second argument of .nops if provided, while the loop in writeNopData makes sure each instruction emitted is no longer than the maximum length allowed by the target.

llvm/test/MC/X86/align-branch-bundle.s

9 ↗

(On Diff #275853)

On second thought, I agree that splitting the patch is the better approach in case the multiple-byte support causes any regression. Will address this in the next iteration.

[jcai19]

Address some of @reames's concerns, and rename variables to avoid confusion.

[aganea]

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

1072 ↗

(On Diff #275853)

Any reason for not reusing maxLongNopLength() rather than rewriting the same thing here? https://github.com/llvm/llvm-project/blob/b2eb1c5793d78d70c1223b098aefc87050f69a8c/llvm/lib/Target/X86/X86MCInstLower.cpp#L1085
That function could perhaps be moved to llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h ?

[jcai19]

Fixed an assertion message.

[craig.topper]

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

1072 ↗

(On Diff #275853)

That function can't be moved as is. It uses X86Subtarget which isn't available to MC. It does something different than for 32-bit mode than what is currently in this patch as that causes additional test failures as discussed elsewhere in this review. That function also uses ProcIntelSLM instead of Feature7ByteNOP. And the FeatureFast flags being set assumes FeatureNOPL is set which is backwards of how it should be.

I think the function here is closer to how it should be except for the 32-bit difference.

[jcai19]

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

1072 ↗

(On Diff #275853)

Any reason for not reusing maxLongNopLength() rather than rewriting the same thing here? https://github.com/llvm/llvm-project/blob/b2eb1c5793d78d70c1223b098aefc87050f69a8c/llvm/lib/Target/X86/X86MCInstLower.cpp#L1085

Yes I'm all for merging these two functions although there are some differences on both 32-bit and 64-bit mode that would break some unit tests, such as https://reviews.llvm.org/D82826?id=275853 on 64-bit mode. Maybe we can address that in a separate patch as previously discussed.

That function could perhaps be moved to llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h ?

SG. Will start to work on merging them once this patch is checked in.

[jcai19]

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

1072 ↗

(On Diff #275853)

@craig.topper Okay I missed your comment. Thanks for the clarification.

[jcai19]

@reames @craig.topper Hi just want to double check if there are any additional issues I should address. Thanks.

[MaskRay]

I think I have a very old (I emphasized it again in another comment) comment which isn't addressed (https://reviews.llvm.org/D82826#2126406 )
We should align with GNU as on the largest operand .nops supports.

[jcai19]

In D82826#2154511, @MaskRay wrote:

I think I have a very old (I emphasized it again in another comment) comment which isn't addressed (https://reviews.llvm.org/D82826#2126406 )
We should align with GNU as on the largest operand .nops supports.

I think it's probably better to address that in a separate patch as the this patch is focused on adding support to .nops directive, similar to this comment https://reviews.llvm.org/D82826?id=275853#inline-765965.

[craig.topper]

llvm/include/llvm/MC/MCFragment.h

358 ↗

(On Diff #275853)

Was this comment addressed? I'm not sure what the variable was called when @reames made this comment.

llvm/lib/MC/MCAssembler.cpp

628 ↗

(On Diff #276235)

Should we put Asm.getBackend().getMaximumNopSize() into a variable? We call it 3 times.

634 ↗

(On Diff #276235)

Should we clamp the NopLength if we hit the error case? reportError won't stop immediately. Or are we relying on writeNopData to not exceed the maximum size internally?

637 ↗

(On Diff #276235)

Maybe just use a if statement here for !NopLength

644 ↗

(On Diff #275853)

I think he was refering to writeNopData. I suppose we could add a limit parameter to writeNopData, but every target would need to be updated for it.

llvm/test/MC/X86/x86-directive-nops-errors.s

5 ↗

(On Diff #276235)

Please use X86 rather than X32. X32 gets confusing with gnux32 where 32-bit pointers are used on a 64-bit target.

[jcai19]

llvm/include/llvm/MC/MCFragment.h

358 ↗

(On Diff #275853)

It was called NopLength. The name was confusing because I called it NopLength here but the same value MaxNopLength somewhere so @reames suggested me to rename it to MaxNopLength . While working on that I realized this value was meant to specify a soft cap on the size limit of a no-op instruction, i.e. the second argument (control) as https://sourceware.org/binutils/docs/as/Nops.html#Nops specified, so I kept the name to avoid the confusion with the function name getMaximumNopSize, which returns a "hard cap" LLVM can accept. Any suggestion on what a better name could be? Also I realized MaxNopLen in the newly introduced emitNops function should be renamed once we make a final decision on the naming. Will address other comments together in the next iteration. Thanks!

[jcai19]

Renamed a variable name and addressed comments. @craig.topper Please let me know if it looks any better. And thanks for enabling multibyte-NOPs in 64-bit mode.

[craig.topper]

LGTM

[jcai19]

In D82826#2189118, @craig.topper wrote:

LGTM

Thank you!

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCInstBuilder.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCParser/MCAsmLexer.h"
 #include "llvm/MC/MCParser/MCAsmParser.h"
 #include "llvm/MC/MCParser/MCParsedAsmOperand.h"
 #include "llvm/MC/MCParser/MCTargetAsmParser.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSection.h"
 #include "llvm/MC/MCStreamer.h"
private:
 
   bool ParseIntelMemoryOperandSize(unsigned &Size);
   std::unique_ptr<X86Operand>
   CreateMemForMSInlineAsm(unsigned SegReg, const MCExpr *Disp, unsigned BaseReg,
                           unsigned IndexReg, unsigned Scale, SMLoc Start,
                           SMLoc End, unsigned Size, StringRef Identifier,
                           const InlineAsmIdentifierInfo &Info);
 
+  bool parseDirectiveNops(SMLoc L);
   bool parseDirectiveEven(SMLoc L);
   bool ParseDirectiveCode(StringRef IDVal, SMLoc L);
 
   /// CodeView FPO data directives.
   bool parseDirectiveFPOProc(SMLoc L);
   bool parseDirectiveFPOSetFrame(SMLoc L);
   bool parseDirectiveFPOPushReg(SMLoc L);
   bool parseDirectiveFPOStackAlloc(SMLoc L);
if (getLexer().isNot(AsmToken::EndOfStatement)) {
       if (Parser.getTok().getString() == "noprefix")
         Parser.Lex();
       else if (Parser.getTok().getString() == "prefix")
         return Error(DirectiveID.getLoc(), "'.intel_syntax prefix' is not "
                                            "supported: registers must not have "
                                            "a '%' prefix in .intel_syntax");
     }
     return false;
-  } else if (IDVal == ".even")
+  } else if (IDVal == ".nops")
+    return parseDirectiveNops(DirectiveID.getLoc());
+  else if (IDVal == ".even")
     return parseDirectiveEven(DirectiveID.getLoc());
   else if (IDVal == ".cv_fpo_proc")
     return parseDirectiveFPOProc(DirectiveID.getLoc());
   else if (IDVal == ".cv_fpo_setframe")
     return parseDirectiveFPOSetFrame(DirectiveID.getLoc());
   else if (IDVal == ".cv_fpo_pushreg")
     return parseDirectiveFPOPushReg(DirectiveID.getLoc());
   else if (IDVal == ".cv_fpo_stackalloc")
bool X86AsmParser::ParseDirective(AsmToken DirectiveID) {
   else if (IDVal == ".seh_savexmm")
     return parseDirectiveSEHSaveXMM(DirectiveID.getLoc());
   else if (IDVal == ".seh_pushframe")
     return parseDirectiveSEHPushFrame(DirectiveID.getLoc());
 
   return true;
 }
 
+/// Return the longest nop which can be efficiently decoded for the given
+/// target cpu.  15-bytes is the longest single NOP instruction, but some
+/// platforms can't decode the longest forms efficiently.
+static unsigned maxLongNopLength(const MCSubtargetInfo &STI) {
+  if (STI.getFeatureBits()[X86::ProcIntelSLM])
+    return 7;
+  if (STI.getFeatureBits()[X86::FeatureFast15ByteNOP])
+    return 15;
+  if (STI.getFeatureBits()[X86::FeatureFast11ByteNOP])
+    return 11;
+  if (STI.getFeatureBits()[X86::FeatureNOPL] ||
+      STI.getFeatureBits()[X86::Mode64Bit])
+    return 10;
+  if (STI.getFeatureBits()[X86::Mode32Bit])
+    return 2;
+  return 1;
+}
+
+/// Emit the largest nop instruction smaller than or equal to \p NumBytes
+/// bytes.  Return the size of nop emitted.
+static unsigned emitNop(MCStreamer &OS, unsigned NumBytes,
+                        const MCSubtargetInfo &STI) {
+  // Cap a single nop emission at the profitable value for the target
+
+  unsigned NopSize;
+  unsigned Opc, BaseReg, ScaleVal, IndexReg, Displacement, SegmentReg;
+  IndexReg = Displacement = SegmentReg = 0;
+  BaseReg = X86::RAX;
+  ScaleVal = 1;
+  switch (NumBytes) {
+  case 0:
+    llvm_unreachable("Zero nops?");
+    break;
+  case 1:
+    NopSize = 1;
+    Opc = X86::NOOP;
+    break;
+  case 2:
+    NopSize = 2;
+    Opc = X86::XCHG16ar;
+    break;
+  case 3:
+    NopSize = 3;
+    Opc = X86::NOOPL;
+    break;
+  case 4:
+    NopSize = 4;
+    Opc = X86::NOOPL;
+    Displacement = 8;
+    break;
+  case 5:
+    NopSize = 5;
+    Opc = X86::NOOPL;
+    Displacement = 8;
+    IndexReg = X86::RAX;
+    break;
+  case 6:
+    NopSize = 6;
+    Opc = X86::NOOPW;
+    Displacement = 8;
+    IndexReg = X86::RAX;
+    break;
+  case 7:
+    NopSize = 7;
+    Opc = X86::NOOPL;
+    Displacement = 512;
+    break;
+  case 8:
+    NopSize = 8;
+    Opc = X86::NOOPL;
+    Displacement = 512;
+    IndexReg = X86::RAX;
+    break;
+  case 9:
+    NopSize = 9;
+    Opc = X86::NOOPW;
+    Displacement = 512;
+    IndexReg = X86::RAX;
+    break;
+  default:
+    NopSize = 10;
+    Opc = X86::NOOPW;
+    Displacement = 512;
+    IndexReg = X86::RAX;
+    SegmentReg = X86::CS;
+    break;
+  }
+
+  unsigned NumPrefixes = std::min(NumBytes - NopSize, 5U);
+  NopSize += NumPrefixes;
+  for (unsigned i = 0; i != NumPrefixes; ++i)
+    OS.emitBytes("\x66");
+
+  switch (Opc) {
+  default:
+    llvm_unreachable("Unexpected opcode");
+  case X86::NOOP:
+    OS.emitInstruction(MCInstBuilder(Opc), STI);
+    break;
+  case X86::XCHG16ar:
+    OS.emitInstruction(MCInstBuilder(Opc).addReg(X86::AX).addReg(X86::AX), STI);
+    break;
+  case X86::NOOPL:
+  case X86::NOOPW:
+    OS.emitInstruction(MCInstBuilder(Opc)
+                           .addReg(BaseReg)
+                           .addImm(ScaleVal)
+                           .addReg(IndexReg)
+                           .addImm(Displacement)
+                           .addReg(SegmentReg),
+                       STI);
+    break;
+  }
+  assert(NopSize <= NumBytes && "We overemitted?");
+  return NopSize;
+}
+
+/// parseDirectiveNops
+///  ::= .nops size[, control]
+bool X86AsmParser::parseDirectiveNops(SMLoc L) {
+  int64_t NumBytes = 0, Control = 0;
+  SMLoc ControlLoc;
+  const MCSubtargetInfo STI = getSTI();
+  if (getParser().checkForValidSection() ||
+      getParser().parseAbsoluteExpression(NumBytes))
+    return true;
+
+  if (parseOptionalToken(AsmToken::Comma)) {
+    ControlLoc = getTok().getLoc();
+    if (getParser().parseAbsoluteExpression(Control))
+      return true;
+  }
+  if (getParser().parseToken(AsmToken::EndOfStatement,
+                             "unexpected token in '.nops' directive"))
+    return true;
+
+  if (NumBytes <= 0) {
+    Warning(L, "'.nops' directive with non-positive size has no effect");
+    return false;
+  }
+
+  if (Control < 0 || Control > maxLongNopLength(STI)) {
+    Warning(ControlLoc, "'.nops' directive with invalid single nop size " +
+                            std::to_string(Control) +
+                            ". (expected within [0, " +
+                            std::to_string(maxLongNopLength(STI)) + "])");
+    return true;
+  }
+
+  if (!Control)
+    Control = maxLongNopLength(STI);
+
+  /// Emit nops
+  unsigned NopsToEmit = NumBytes;
+  //(void)NopsToEmit;
+  while (NumBytes) {
+    NumBytes -=
+        emitNop(getParser().getStreamer(), std::min(NumBytes, Control), STI);
+    assert(NopsToEmit >= NumBytes && "Emitted more than I asked for!");
+  }
+
+  return false;
+}
+
 /// parseDirectiveEven
 ///  ::= .even
 bool X86AsmParser::parseDirectiveEven(SMLoc L) {
   if (parseToken(AsmToken::EndOfStatement, "unexpected token in directive"))
     return false;
 
   const MCSection *Section = getStreamer().getCurrentSectionOnly();
   if (!Section) {

llvm/test/MC/X86/x86-directive-nops.s

+// RUN: llvm-mc -triple i386 %s -filetype=obj 2>&1 -o %t | FileCheck --check-prefix=X32-WARNINGS %s
+// RUN: llvm-objdump -d %t | FileCheck --check-prefix=X32 %s
+// RUN: llvm-mc -triple x86_64 %s -filetype=obj 2>&1 -o %t | FileCheck --check-prefix=X64-WARNINGS %s
+// RUN: llvm-objdump -d %t | FileCheck --check-prefix=X64 %s
+
+.nops 4, 1
+#X32:       0: 90 nop
+#X32-NEXT:  1: 90 nop
+#X32-NEXT:  2: 90 nop
+#X32-NEXT:  3: 90 nop
+#X64:       0: 90 nop
+#X64-NEXT:  1: 90 nop
+#X64-NEXT:  2: 90 nop
+#X64-NEXT:  3: 90 nop
+.nops 4, 2
+#X32-NEXT:  4: 66 90 nop
+#X32-NEXT:  6: 66 90 nop
+#X64-NEXT:  4: 66 90 nop
+#X64-NEXT:  6: 66 90 nop
+.nops 4, 3
+#X32-WARNINGS: :[[@LINE-1]]:10: warning: '.nops' directive with invalid single nop size 3.
+#X64-NEXT:  8: 0f 1f 00 nopl (%rax)
+#X64-NEXT:  b: 90 nop
+.nops 4, 4
+#X32-WARNINGS: :[[@LINE-1]]:10: warning: '.nops' directive with invalid single nop size 4.
+#X64:       c: 0f 1f 40 08 nopl 8(%rax)
+.nops 4, 5
+#X32-WARNINGS: :[[@LINE-1]]:10: warning: '.nops' directive with invalid single nop size 5.
+#X64:      10: 0f 1f 40 08 nopl 8(%rax)
+.nops 16, 15
+#X32-WARNINGS: :[[@LINE-1]]:11: warning: '.nops' directive with invalid single nop size 15.
+#X64-WARNINGS: :[[@LINE-2]]:11: warning: '.nops' directive with invalid single nop size 15.