This is an archive of the discontinued LLVM Phabricator instance.

[X86][RFC] Support new feature AVX10
Changes PlannedPublic

Authored by pengfei on Aug 9 2023, 3:06 AM.

Diff Detail

Event Timeline

pengfei created this revision.Aug 9 2023, 3:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2023, 3:06 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
pengfei requested review of this revision.Aug 9 2023, 3:06 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 9 2023, 3:06 AM
pengfei edited the summary of this revision. (Show Details)Aug 9 2023, 3:08 AM
goldstein.w.n added inline comments.
clang/lib/Basic/Targets/X86.h
99

Maybe should be HasAVX10_1_512? As brought up the rfc, there might be an avx10.2-512

Likewise elsewhere, or is this to match GCC?

Matt added a subscriber: Matt.Aug 9 2023, 3:10 PM
pengfei added inline comments.Aug 10 2023, 7:23 AM
clang/lib/Basic/Targets/X86.h
99

No. HasAVX10_512BIT is a single feature which can be combined with HasAVX10_1, HasAVX10_2 etc.
AFAIK, GCC chooses the similar idea here.

Ping~ It looks to me there's no concern about this solution in the RFC. I think we can move forward to land it.

skan added inline comments.Aug 15 2023, 11:33 PM
clang/lib/CodeGen/CodeGenFunction.cpp
2581

Minor suggestion. The code here may be refined to be better extended by other targets, like

llvm::Triple::ArchType ArchType =
    getContext().getTargetInfo().getTriple().getArch();

switch (ArchType) {
case llvm::Triple::x86:
case llvm::Triple::x86_64: {
   ....
 }
 default:
  return;
llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
924

I think what you need here is TSFlags & X86II::EVEX_L2 instead of getL2(). The class X86OpcodePrefixHelper is designed for encoding only. The bit L2 can be set in other cases so it may blur the meaning of 512-bit here you use the getter.

926

-mavx10.1 does not work for assembler. So if such instruction is generated w/o AVX10-512BIT support, it must be compiler's issue instead of user's. An assert should be more appropriate here.

skan added inline comments.Aug 15 2023, 11:54 PM
llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
926

-mavx10.1 does not work for assembler. So if such instruction is generated w/o AVX10-512BIT support, it must be compiler's issue instead of user's. An assert should be more appropriate here.

Reference: https://llvm.org/docs/CodingStandards.html#assert-liberally

pengfei updated this revision to Diff 550669.Aug 16 2023, 2:13 AM
pengfei marked an inline comment as done.

Address comment.

clang/lib/CodeGen/CodeGenFunction.cpp
2581

We have a few place code using isX86. I think it's more convenient to use a single condition. We can refactor when necessary.

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
926

We need to report fatal error for this case even if it's a compiler bug. Otherwise, user may observe the crash issue in runtime and hard to find the reason.

skan accepted this revision.Aug 16 2023, 6:17 PM

LGTM

This revision is now accepted and ready to land.Aug 16 2023, 6:17 PM
MaskRay added inline comments.Aug 16 2023, 6:26 PM
clang/lib/Basic/Targets/X86.cpp
739

This is untested?

MaskRay added inline comments.Aug 16 2023, 7:22 PM
clang/lib/Driver/ToolChains/Arch/X86.cpp
261

warn_drv_overriding_flag_option is under the group -Woverriding-t-option, which was intended for clang-cl /T* options (D1290).

I created D158137 to add -Woverriding-option.

e-kud added a comment.Aug 17 2023, 7:46 PM

Just curious, in RFC we have -mavx10.x-256/-mavx10.x-512 but here we refer to -mavx10.x/-mavx10.x,-mavx10-512bit. Is it compliant with GCC, or the revision is just for the illustrative purpose?

pengfei planned changes to this revision.Aug 31 2023, 12:00 AM

Just curious, in RFC we have -mavx10.x-256/-mavx10.x-512 but here we refer to -mavx10.x/-mavx10.x,-mavx10-512bit. Is it compliant with GCC, or the revision is just for the illustrative purpose?

Sorry for the late reply. We have received a couple concerns about how to interpret these options, especially when used together with AVX512 options. We decided not to provide AVX10.1 options at the present, instead, we just provide -m[no-]evex512 to disable ZMM and 64-bit mask instructions for AVX512 features. For more details, lease take a look at D159250.

clang/lib/Basic/Targets/X86.cpp
739

Done in an alternative reversion D159250.

clang/lib/Driver/ToolChains/Arch/X86.cpp
261

Thanks! This is not needed in the new version.