[PATCH] D97268: [X86] Use correct padding when in 16-bit mode

[James Y Knight via Phabricator via llvm-commits]

jyknight accepted this revision.
jyknight added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:1079
     return 1;
+  if (STI.getFeatureBits()[X86::Mode16Bit])
+    return 4;
----------------
craig.topper wrote:
> void wrote:
> > craig.topper wrote:
> > > Should this just be above the NOPL check if we're going to AND NOPL with !STI.hasFeature(X86::Mode16Bit).
> > > 
> > > Should we consistently use hasFeature in this function instead of mixing with getFeatureBits()?
> > I was told to place this here and not at the start.
> Sorry. I realize that, but that request from James doesn't make sense to me unless James thought the 16-bit NOPs required NOPL. Which they don't. @jyknight can you confirm why you asked for that change?
What I had intended to ask was that with !NOPL, the function should return 1 rather than 4 even in 16-bit mode.

And the reason I asked for that is if we were disabling the 2-byte nop in 32-bit mode, surely we should also be disabling it in 16-bit mode, too -- the 2-byte NOP is the same in the 16 and 32-bit nop lists. But, looking deeper, this was incorrect, because as far as I can tell there was never a reason for the code to be disabling 0x66 0x90 for !NOPL in the first place.

So, we could've/should've been returning at least 2 even with the current single table. 

We could also provide a 3rd version of the nop-table for !16bit && !NOPL && !X86_64, having `lea 0(%esi),%esi` and `lea 0(%esi,1),%esi` as the 3/4-byte NOPs. I don't know if that'd be a good idea though -- I could imagine that using 3/4-byte LEA pseudo-NOPs instead of 1/2-byte actual NOPs might marginally degrade performance when people are compiling for generic-i686 and running on a modern CPU -- even though it does result in fewer instructions.

So, upshot: yep, just put this back the way you had it before. (And if you want to tackle the other issues too, that'd be fine, but also fine if not.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97268