Skip to content

[Driver][X86] Add -m[no-]apxf to m_x86_Features_Group #140874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fzou1 merged 4 commits into from
May 26, 2025
Merged

[Driver][X86] Add -m[no-]apxf to m_x86_Features_Group #140874

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d683302
[Driver][X86] Add -m[no-]apxf to m_x86_Features_Group
fzou1 May 20, 2025
c845e22
Remove help text for m[no_]apxf.
fzou1 May 23, 2025
213f584
Removed some test lines to save run time for LIT tests
fzou1 May 23, 2025
ec996f9
Removed "-o t.o"
fzou1 May 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6679,8 +6679,13 @@ def mapx_features_EQ : CommaJoined<["-"], "mapx-features=">, Group<m_x86_Feature
HelpText<"Enable features of APX">, Values<"egpr,push2pop2,ppx,ndd,ccmp,nf,cf,zu">, Visibility<[ClangOption, CLOption, FlangOption]>;
def mno_apx_features_EQ : CommaJoined<["-"], "mno-apx-features=">, Group<m_x86_Features_Group>,
HelpText<"Disable features of APX">, Values<"egpr,push2pop2,ppx,ndd,ccmp,nf,cf,zu">, Visibility<[ClangOption, CLOption, FlangOption]>;
def mapxf : Flag<["-"], "mapxf">, Alias<mapx_features_EQ>, AliasArgs<["egpr","push2pop2","ppx","ndd","ccmp","nf","cf","zu"]>;
def mno_apxf : Flag<["-"], "mno-apxf">, Alias<mno_apx_features_EQ>, AliasArgs<["egpr","push2pop2","ppx","ndd","ccmp","nf","cf","zu"]>;
def mapxf : Flag<["-"], "mapxf">, Alias<mapx_features_EQ>,
AliasArgs<["egpr","push2pop2","ppx","ndd","ccmp","nf","cf","zu"]>,
Group<m_x86_Features_Group>, HelpText<"Enable all features of APX">;
def mno_apxf : Flag<["-"], "mno-apxf">, Alias<mno_apx_features_EQ>,
AliasArgs<["egpr","push2pop2","ppx","ndd","ccmp","nf","cf","zu"]>,
Group<m_x86_Features_Group>,
HelpText<"Disable all features of APX">;
Copy link
Contributor

@KanRobert KanRobert May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description is not accurate. APX -> APX_F.

BTW, I suggest to remove the HelpText for m[no-]apxf since other features has no text either, so that they would not appear in --help

Copy link
Contributor Author

@fzou1 fzou1 May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Is it expected not to show the feature option in --help? Why?

Copy link
Contributor

@KanRobert KanRobert May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear about that, it's current convention. Probably -march/mcpu is preferred.

Copy link
Contributor Author

@fzou1 fzou1 May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Thanks.

def mapx_inline_asm_use_gpr32 : Flag<["-"], "mapx-inline-asm-use-gpr32">, Group<m_Group>,
HelpText<"Enable use of GPR32 in inline assembly for APX">;
} // let Flags = [TargetSpecific]
Expand Down
50 changes: 50 additions & 0 deletions clang/test/Driver/cl-x86-flags.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,3 +135,53 @@

void f(void) {
}


// RUN: not %clang_cl -### --target=i386 -mapx-features=ndd %s 2>&1 | FileCheck --check-prefix=NON-APX %s
// RUN: not %clang_cl -### --target=i386 -mapxf %s 2>&1 | FileCheck --check-prefix=NON-APX %s
// RUN: %clang_cl -### --target=i386 -mno-apxf %s 2>&1 > /dev/null
// NON-APX: error: unsupported option '-mapx-features=|-mapxf' for target 'i386'
// NON-APX-NOT: error: {{.*}} -mapx-features=

// RUN: %clang_cl -target x86_64-pc-windows -mapxf %s -### -o %t.o 2>&1 | FileCheck -check-prefix=APXF %s
Copy link
Member

@MaskRay MaskRay May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't test the 4 combinations. We usually just test -mapxf and -mapxf -mno-apxf. Although this leaves some feature coverage gaps, it's a compromise to avoid adding too many RUN lines.

Copy link
Contributor Author

@fzou1 fzou1 May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Updated.

// RUN: %clang_cl -target x86_64-pc-windows -mno-apxf %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-APXF %s
// RUN: %clang_cl -target x86_64-pc-windows -mno-apxf -mapxf %s -### -o %t.o 2>&1 | FileCheck -check-prefix=APXF %s
// RUN: %clang_cl -target x86_64-pc-windows -mapxf -mno-apxf %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-APXF %s
//
// APXF: "-target-feature" "+egpr" "-target-feature" "+push2pop2" "-target-feature" "+ppx" "-target-feature" "+ndd" "-target-feature" "+ccmp" "-target-feature" "+nf" "-target-feature" "+cf" "-target-feature" "+zu"
// NO-APXF: "-target-feature" "-egpr" "-target-feature" "-push2pop2" "-target-feature" "-ppx" "-target-feature" "-ndd" "-target-feature" "-ccmp" "-target-feature" "-nf" "-target-feature" "-cf" "-target-feature" "-zu"

// RUN: %clang_cl -target x86_64-pc-windows -mapx-features=egpr %s -### -o %t.o 2>&1 | FileCheck -check-prefix=EGPR %s
Copy link
Member

@MaskRay MaskRay May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-target has been deprecated around clang 3.4. Use --target= in new tests.

Do we really need so many RUN lines? They add up and make test execution slower.

Copy link
Contributor

@KanRobert KanRobert May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we only test -m[no-]apxf/, -mapx-features are usable for clang-cl? Since other tests are already tested on clang side. @fzou1

Copy link
Contributor Author

@fzou1 fzou1 May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Removed some RUN lines.

// RUN: %clang_cl -target x86_64-pc-windows -mapx-features=push2pop2 %s -### -o %t.o 2>&1 | FileCheck -check-prefix=PUSH2POP2 %s
// RUN: %clang_cl -target x86_64-pc-windows -mapx-features=ppx %s -### -o %t.o 2>&1 | FileCheck -check-prefix=PPX %s
// RUN: %clang_cl -target x86_64-pc-windows -mapx-features=ndd %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NDD %s
// RUN: %clang_cl -target x86_64-pc-windows -mapx-features=ccmp %s -### -o %t.o 2>&1 | FileCheck -check-prefix=CCMP %s
// RUN: %clang_cl -target x86_64-pc-windows -mapx-features=nf %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NF %s
// RUN: %clang_cl -target x86_64-pc-windows -mapx-features=cf %s -### -o %t.o 2>&1 | FileCheck -check-prefix=CF %s
// RUN: %clang_cl -target x86_64-pc-windows -mapx-features=zu %s -### -o %t.o 2>&1 | FileCheck -check-prefix=ZU %s
// EGPR: "-target-feature" "+egpr"
// PUSH2POP2: "-target-feature" "+push2pop2"
// PPX: "-target-feature" "+ppx"
// NDD: "-target-feature" "+ndd"
// CCMP: "-target-feature" "+ccmp"
// NF: "-target-feature" "+nf"
// CF: "-target-feature" "+cf"
// ZU: "-target-feature" "+zu"

// RUN: %clang_cl -target x86_64-pc-windows -mapx-features=egpr,ndd %s -### -o %t.o 2>&1 | FileCheck -check-prefix=EGPR-NDD %s
// RUN: %clang_cl -target x86_64-pc-windows -mapx-features=egpr -mapx-features=ndd %s -### -o %t.o 2>&1 | FileCheck -check-prefix=EGPR-NDD %s
// RUN: %clang_cl -target x86_64-pc-windows -mno-apx-features=egpr -mno-apx-features=ndd %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-EGPR-NO-NDD %s
// RUN: %clang_cl -target x86_64-pc-windows -mno-apx-features=egpr -mapx-features=egpr,ndd %s -### -o %t.o 2>&1 | FileCheck -check-prefix=EGPR-NDD %s
// RUN: %clang_cl -target x86_64-pc-windows -mno-apx-features=egpr,ndd -mapx-features=egpr %s -### -o %t.o 2>&1 | FileCheck -check-prefix=EGPR-NO-NDD %s
// RUN: %clang_cl -target x86_64-pc-windows -mapx-features=egpr,ndd -mno-apx-features=egpr %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-EGPR-NDD %s
// RUN: %clang_cl -target x86_64-pc-windows -mapx-features=egpr -mno-apx-features=egpr,ndd %s -### -o %t.o 2>&1 | FileCheck -check-prefix=NO-EGPR-NO-NDD %s
//
// EGPR-NDD: "-target-feature" "+egpr" "-target-feature" "+ndd"
// EGPR-NO-NDD: "-target-feature" "-ndd" "-target-feature" "+egpr"
// NO-EGPR-NDD: "-target-feature" "+ndd" "-target-feature" "-egpr"
// NO-EGPR-NO-NDD: "-target-feature" "-egpr" "-target-feature" "-ndd"

// RUN: not %clang_cl -target x86_64-pc-windows -mapx-features=egpr,foo,bar %s -### -o %t.o 2>&1 | FileCheck -check-prefix=ERROR %s
//
// ERROR: unsupported argument 'foo' to option '-mapx-features='
Copy link
Member

@MaskRay MaskRay May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error check patterns should always include error:

remove unneeded -o %t.o

Copy link
Contributor Author

@fzou1 fzou1 May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// ERROR: unsupported argument 'bar' to option '-mapx-features='
Loading