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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:1072
 
+unsigned X86AsmBackend::getMaximumNopSize() const {
+  if (!STI.getFeatureBits()[X86::FeatureNOPL] &&
----------------
aganea wrote:
> reames wrote:
> > 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.
> Any reason for not reusing `maxLongNopLength()` rather than rewriting the same thing here? https://github.com/llvm/llvm-project/blob/b2eb1c5793d78d70c1223b098aefc87050f69a8c/llvm/lib/Target/X86/X86MCInstLower.cpp#L1085
> That function could perhaps be moved to `llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h` ?
> 
That function can't be moved as is. It uses X86Subtarget which isn't available to MC. It does something different than for 32-bit mode than what is currently in this patch as that causes additional test failures as discussed elsewhere in this review. That function also uses ProcIntelSLM instead of Feature7ByteNOP. And the FeatureFast flags being set assumes FeatureNOPL is set which is backwards of how it should be.

I think the function here is closer to how it should be except for the 32-bit difference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82826