This is an archive of the discontinued LLVM Phabricator instance.

[X86] Rename FeatureCMPXCHG8B/FeatureCMPXCHG16B to FeatureCX8/CX16 to match CPUID.
ClosedPublic

Authored by craig.topper on Mar 17 2022, 11:19 PM.

Details

Summary

Rename hasCMPXCHG16B() to useCMPXCHG16B() to make it less like other
feature functions.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 17 2022, 11:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2022, 11:19 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
craig.topper requested review of this revision.Mar 17 2022, 11:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2022, 11:19 PM
craig.topper added inline comments.Mar 17 2022, 11:21 PM
llvm/lib/Target/X86/X86InstrInfo.td
2238

I used hasCX16 here instead of useCMPXCHG16B since we have to use In64BitMode explicitly any way for the assembler.

skan accepted this revision.Mar 17 2022, 11:29 PM

LGTM

This revision is now accepted and ready to land.Mar 17 2022, 11:29 PM
skan added inline comments.Mar 18 2022, 12:06 AM
llvm/lib/Target/X86/X86Subtarget.h
717–719

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.

craig.topper added inline comments.Mar 18 2022, 12:34 AM
llvm/lib/Target/X86/X86Subtarget.h
717–719

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.

skan added inline comments.Mar 18 2022, 1:20 AM
llvm/lib/Target/X86/X86Subtarget.h
717–719

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.

skan added a comment.EditedMar 19 2022, 2:57 AM

LGTM. I have no concern for this patch now.

Use canUseCMPXCHG8B/16B()

This revision was landed with ongoing or failed builds.Mar 19 2022, 12:39 PM
This revision was automatically updated to reflect the committed changes.