[PATCH] D121978: [X86] Rename FeatureCMPXCHG8B/FeatureCMPXCHG16B to FeatureCX8/CX16 to match CPUID.

[Kan Shengchen via Phabricator via llvm-commits]

skan added inline comments.


================
Comment at: llvm/lib/Target/X86/X86Subtarget.h:717-719
+    // CX16 is just the CPUID bit, the instruction also requires 64-bit mode.
+    return HasCX16 && is64Bit();
+  }
----------------
craig.topper wrote:
> skan wrote:
> > I have a little concern about the name:
> > 1.  `CMPXCHG16B`<-> `CX16` One use abbreviation but the other not, a little confusing
> > 2.  `useCMPXCHG16B` is not so reasonable, the function is used to check if we //could// use CX16
> > 
> > I'm afraid adding `useCMPXCHG16B` would make `hasCX16` useless.
> I should probably use hasCX16() in useCMPXCHG16B() instead of HasCX16. That would match with is64Bit().
> 
> useCMPXCHG16B name is kind of like the existing UseSSE and UseAVX predicates. Would canUseCMPXCHG16B or canUseCX16 be better?
> 
> Ultimately my goal is to get rid of the concept of a non-trivial feature from the other patch. The Subtarget interface is bad right now, and it’s mostly that way because naming things is hard.
> 
> I’m a little tempted to remove useCMPXCHG16B and just put the is64Bit check at the callers.
> I should probably use hasCX16() in useCMPXCHG16B() instead of HasCX16. That would match with is64Bit().
> 
> useCMPXCHG16B name is kind of like the existing UseSSE and UseAVX predicates. Would canUseCMPXCHG16B or canUseCX16 be better?
> 

Okay, then `UseCX16` makes sense for me.

> Ultimately my goal is to get rid of the concept of a non-trivial feature from the other patch. The Subtarget interface is bad right now, and it’s mostly that way because naming things is hard.
> 

This method can not get rid of non-trivial feature like `useAA`, which has an virtual interface.  I think non-trivial feature brings the flexibility, which gives the developer chance to hack the code quickly and see the difference, so it's important.

> I’m a little tempted to remove useCMPXCHG16B and just put the is64Bit check at the callers.

This brings burdon to the user, which is worrying.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121978