[PATCH] D81301: [X86] Emit two-byte NOP when possible

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1102
+    if (Subtarget->is32Bit() && NumBytes >= 2) {
+      OS.emitInstruction(
+          MCInstBuilder(X86::XCHG16ar).addReg(X86::AX).addReg(X86::AX),
----------------
craig.topper wrote:
> craig.topper wrote:
> > efriedma wrote:
> > > MaskRay wrote:
> > > > Can the x86-64 code path below be reused?
> > > I see three issues with using the 64-bit codepath on 32-bit:
> > > 
> > > 1. We can't use the patterns based on REX prefixes.
> > > 2. The X86::NOOPL opcode requires FeatureNOPL.
> > > 3. If Mode16Bit is enabled, XCHG16ar is actually a one-byte instruction.
> > > 
> > > So we could sort of reuse the code below, but it would require a lot of refactoring.
> > Can Mode16Bit be set here? We're coming from MachineInstr and we don't compile to 16 bit do we?
> > 
> > The only part in the reset of the code that is truly 64-bit specific is the IndexReg being RAX. There's nothing in there that requiers a REX prefix.
> > 
> > So it seems like we just need to replace the Is64Bit check we had before with FeatureNOPL and then pick the index register as RAX or EAX depending on 64 bit or 32 bit.
> I guess we'd still need to handle the case where the CPU was set to a 386/486/pentium where FeatureNOPL isn't set.
> Can Mode16Bit be set here? We're coming from MachineInstr and we don't compile to 16 bit do we?

We support generating "16-bit" code, yes, if you specify `-m16` to clang.  (Semantically, it's actually 32-bit code with a bunch of size prefixes to allow running it in 16-bit mode, but that doesn't really matter from the assembler's perspective.)

> then pick the index register as RAX or EAX depending on 64 bit or 32 bit.

We can't just replace RAX with EAX; that would reduce the length of the opcode by one.  We'd actually need to pick different encodings in the cases where that's relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81301