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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/MC/MCFragment.h:358
+  /// Maximum number of bytes of each instruction.
+  int64_t NopLength;
+
----------------
Can you call this MaxNopLength or something?


================
Comment at: llvm/lib/MC/MCAssembler.cpp:625
+
+    if (NumBytes < 0) {
+      Asm.getContext().reportError(
----------------
When parsing asm, you reject negative lengths.  Should these simply be asserts?


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


================
Comment at: llvm/lib/MC/MCAssembler.cpp:644
+
+    while (NumBytes) {
+      uint64_t NopsToEmit = (uint64_t)std::min(NumBytes, MaxNopLength);
----------------
This loop is duplicated from within emitNops.  Can you pass in a MaxNopLength parameter instead?


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:1072
 
+unsigned X86AsmBackend::getMaximumNopSize() const {
+  if (!STI.getFeatureBits()[X86::FeatureNOPL] &&
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82826