[PATCH] D82826: [X86] support .nops directive

[Jian Cai via Phabricator via llvm-commits]

jcai19 marked 2 inline comments as done.
jcai19 added inline comments.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:633
+    if (MaxNopLength < 0 ||
+        MaxNopLength > Asm.getBackend().getMaximumNopSize()) {
+      Asm.getContext().reportError(
----------------
reames wrote:
> 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*.  
> 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
^



================
Comment at: llvm/test/MC/X86/align-branch-bundle.s:9
 # CHECK-NEXT:       e:       nop
-# CHECK-NEXT:       f:       nop
 # CHECK-NEXT:      10:       jle
 
----------------
reames wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82826/new/

https://reviews.llvm.org/D82826