[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:644
+
+    while (NumBytes) {
+      uint64_t NopsToEmit = (uint64_t)std::min(NumBytes, MaxNopLength);
----------------
reames wrote:
> This loop is duplicated from within emitNops.  Can you pass in a MaxNopLength parameter instead?
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82826