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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/MC/MCFragment.h:358
+  /// Maximum number of bytes of each instruction.
+  int64_t NopLength;
+
----------------
reames wrote:
> Can you call this MaxNopLength or something?
Was this comment addressed? I'm not sure what the variable was called when @reames made this comment.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:644
+
+    while (NumBytes) {
+      uint64_t NopsToEmit = (uint64_t)std::min(NumBytes, MaxNopLength);
----------------
jcai19 wrote:
> 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.
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.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:628
+
+    if (NopLength > Asm.getBackend().getMaximumNopSize()) {
+      Asm.getContext().reportError(
----------------
Should we put Asm.getBackend().getMaximumNopSize() into a variable? We call it 3 times.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:634
+              std::to_string(Asm.getBackend().getMaximumNopSize()) + "])");
+    }
+
----------------
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?


================
Comment at: llvm/lib/MC/MCAssembler.cpp:637
+    // Use maximum value if the size of each NOP is not specified
+    NopLength = !NopLength ? Asm.getBackend().getMaximumNopSize() : NopLength;
+
----------------
Maybe just use a if statement here for !NopLength


================
Comment at: llvm/test/MC/X86/x86-directive-nops-errors.s:5
+.nops 4, 3
+# X32: :[[@LINE-1]]:1: error: illegal NOP size 3.
+.nops 4, 4
----------------
Please use X86 rather than X32.   X32 gets confusing with gnux32 where 32-bit pointers are used on a 64-bit target.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82826