[PATCH] D17573: [X86] PR26554: Enable using of true long nops for x86-64 for every CPU

[Andrey Turetskiy via llvm-commits]

aturetsk added inline comments.

================
Comment at: lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:353
@@ -345,3 +352,3 @@
   // Select the right NOP table.
   // FIXME: Can we get if CPU supports long nops from the subtarget somehow?
   const uint8_t (*Nops)[10] = HasNopl ? TrueNops : AltNops;
----------------
RKSimon wrote:
> bruno wrote:
> > aturetsk wrote:
> > > bruno wrote:
> > > > Any reason why this shouldn't be done as a target feature as suggested in the FIXME above?
> > > Target feature can be used as indicator of true long nop support, but that won't fix the issue.
> > > 
> > > The alternative nop instructions I introduced in http://reviews.llvm.org/D14178 are not actually nops for x86-64 arch. But all CPUs which don't support true long nops are 32-bit-only. So I propose to use true long nops if we compile for x86-64 arch for any CPU even if it's marked as 'doesn't support true long nops'.
> > > Well, we do generate 64-bit instructions for 32-bit-only CPUs when x86-64 arch is used, why not generate true long nops as well?
> > > And I think 'generic' should use true long nops in 64-bit mode (since every real 64 bit CPU seems to support them) and use alternative long nops in 32 bit mode.
> > I see. However, we could have the opposite logic, something like "FeatureShortNop", and mark generic, i386, etc with them. I looked up these CPUs and none of them have Feature64Bit, so guess HasNopl will simply become !FeatureShortNop.
> Basing this off target features would also make it much easier to chose the optimal nop length for intel/amd (as discussed on PR22965).
Oh, I think my understanding of 'generic' CPU wasn't quite right. It's not supposed to be used in 64-bit mode, right? ('x86-64' CPU seems to be 64-bit 'generic')
If that's so then I see no reason to avoid using target features and I'll rework the patch (using FeatureLongNop).

Simon, how do you suggest to choose the optimal nop length in this case?
Use different target features for different max lenghts like this:
FeatureLongNop - allow nops of any length
FeatureLongNop7 - only allow nops smaller than 7 bytes
... etc?


http://reviews.llvm.org/D17573