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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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();
+  }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121978