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

[Jian Cai via Phabricator via llvm-commits]

jcai19 marked an inline comment as done.
jcai19 added inline comments.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:1072
 
+unsigned X86AsmBackend::getMaximumNopSize() const {
+  if (!STI.getFeatureBits()[X86::FeatureNOPL] &&
----------------
jcai19 wrote:
> craig.topper wrote:
> > 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.
> > 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
> 
> Yes I'm all for merging these two functions although there are some differences on both 32-bit and 64-bit mode that would break some unit tests, such as https://reviews.llvm.org/D82826?id=275853 on 64-bit mode. Maybe we can address that in a separate patch as previously discussed. 
> 
> > That function could perhaps be moved to llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h ?
> 
>  SG. Will start to work on merging them once this patch is checked in.
@craig.topper Okay I missed your comment. Thanks for the clarification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82826