Skip to content

[clang] Support -Wa, options -mmsa and -mno-msa #99615

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
merged 2 commits into from
Aug 20, 2024
Merged

[clang] Support -Wa, options -mmsa and -mno-msa #99615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c83997
[clang] Support -Wa, options -mmsa and -mno-msa
yingopq Jul 19, 2024
073ab2c
Simplify code and test
MaskRay Aug 20, 2024
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
1 change: 1 addition & 0 deletions clang/include/clang/Basic/CodeGenOptions.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ CODEGENOPT(MergeAllConstants , 1, 1) ///< Merge identical constants.
CODEGENOPT(MergeFunctions , 1, 0) ///< Set when -fmerge-functions is enabled.
CODEGENOPT(NoCommon , 1, 0) ///< Set when -fno-common or C++ is enabled.
CODEGENOPT(NoExecStack , 1, 0) ///< Set when -Wa,--noexecstack is enabled.
CODEGENOPT(MipsMsa , 1, 0) ///< Set when -Wa,-mmsa is enabled.
CODEGENOPT(FatalWarnings , 1, 0) ///< Set when -Wa,--fatal-warnings is
///< enabled.
CODEGENOPT(NoWarn , 1, 0) ///< Set when -Wa,--no-warn is enabled.
Expand Down
4 changes: 3 additions & 1 deletion clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5383,7 +5383,9 @@ def mmadd4 : Flag<["-"], "mmadd4">, Group<m_mips_Features_Group>,
def mno_madd4 : Flag<["-"], "mno-madd4">, Group<m_mips_Features_Group>,
HelpText<"Disable the generation of 4-operand madd.s, madd.d and related instructions.">;
def mmsa : Flag<["-"], "mmsa">, Group<m_mips_Features_Group>,
HelpText<"Enable MSA ASE (MIPS only)">;
Visibility<[ClangOption, CC1Option, CC1AsOption]>,
HelpText<"Enable MSA ASE (MIPS only)">,
MarshallingInfoFlag<CodeGenOpts<"MipsMsa">>;
def mno_msa : Flag<["-"], "mno-msa">, Group<m_mips_Features_Group>,
HelpText<"Disable MSA ASE (MIPS only)">;
def mmt : Flag<["-"], "mmt">, Group<m_mips_Features_Group>,
Expand Down
13 changes: 12 additions & 1 deletion clang/lib/Driver/ToolChains/Clang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2556,6 +2556,7 @@ static void CollectArgsForIntegratedAssembler(Compilation &C,
bool Crel = false, ExperimentalCrel = false;
bool UseRelaxRelocations = C.getDefaultToolChain().useRelaxRelocations();
bool UseNoExecStack = false;
bool Msa = false;
Copy link
Member

Choose a reason for hiding this comment

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

std::optional<bool> Msa. -mno-msa should lead to an error even on non-mips targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added bool NoMsa and lead to "unsupported" error when it is on non-mips, were I understanding correctly?

Copy link
Member

Choose a reason for hiding this comment

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

bool Msa, NoMsa encodes 4 states where Msa&&NoMsa is meaningless but not checked.

std::optional<bool> Msa encodes the desired 3 states:

  • not set
  • set and true
  • set and false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I updated.

Copy link
Member

Choose a reason for hiding this comment

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

Please do use std::optional<bool> Msa and please don't add err_drv_msa_and_nomsa. It's common for -Wa,--foo to override a previous -Wa,--no-foo. There shouldn't be an error.

Copy link
Member

Choose a reason for hiding this comment

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

std::optional<bool> Msa

if (-mmsa) Msa = true;
if (-mno-msa) Msa = false;


if (Msa) {
  if (not mips) ...
  else if (*Msa) CmdArgs.push_back("-mmsa");
}

Copy link
Contributor Author

@yingopq yingopq Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

std::optional<bool> Msa. -mno-msa should lead to an error even on non-mips targets.

What error need I add in this 444cd83, and what is *Msa?

Copy link
Contributor

Choose a reason for hiding this comment

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

-mno-msa should lead to an error even on non-mips targets.

Assemblers accept this option, so I don't think we should throw an error when -mno-msa is used with MIPS targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bool Msa, NoMsa encodes 4 states where Msa&&NoMsa is meaningless but not checked.

std::optional<bool> Msa encodes the desired 3 states:

  • not set
  • set and true
  • set and false

Sorry, I didn't realize you were asking me to use an option type bool MSA on the first version of the modification.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW using std::Optional<bool> makes more sense since it does distinguish between None (totally not set) and Boolean false (disabled). Your initial thought is that false stands for 1) MSA flag is not set and 2) MSA is disabled, but if we target a non-MIPS machine, then we must know if the -mno-msa is actually set - that's why using plain bool in this context is not a good idea, false can not mean two states.

const char *MipsTargetFeature = nullptr;
StringRef ImplicitIt;
for (const Arg *A :
Expand Down Expand Up @@ -2665,7 +2666,14 @@ static void CollectArgsForIntegratedAssembler(Compilation &C,
CmdArgs.push_back("-soft-float");
continue;
}

if (Value == "-mmsa") {
Msa = true;
continue;
}
if (Value == "-mno-msa") {
Msa = false;
continue;
}
MipsTargetFeature = llvm::StringSwitch<const char *>(Value)
.Case("-mips1", "+mips1")
.Case("-mips2", "+mips2")
Expand All @@ -2685,6 +2693,7 @@ static void CollectArgsForIntegratedAssembler(Compilation &C,
.Default(nullptr);
if (MipsTargetFeature)
continue;
break;
}

if (Value == "-force_cpusubtype_ALL") {
Expand Down Expand Up @@ -2777,6 +2786,8 @@ static void CollectArgsForIntegratedAssembler(Compilation &C,
<< "-Wa,--crel" << D.getTargetTriple();
}
}
if (Msa)
CmdArgs.push_back("-mmsa");
if (!UseRelaxRelocations)
CmdArgs.push_back("-mrelax-relocations=no");
if (UseNoExecStack)
Expand Down
12 changes: 12 additions & 0 deletions clang/test/Driver/mips-msa.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// RUN: %clang -### -c --target=mips64el-unknown-linux-gnuabi64 \
Copy link
Member

Choose a reason for hiding this comment

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

We don't need -fno-integrated-as tests. They are just forwarded.

The -mmsa test needs a NOT. I'll push a commit to improve the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK,Thanks!

// RUN: -Wa,-mmsa %s -Werror 2>&1 | FileCheck %s --check-prefix=CHECK-MMSA
// CHECK-MMSA: "-cc1" {{.*}}"-mmsa"

// RUN: %clang -### -c --target=mips64el-unknown-linux-gnuabi64 \
// RUN: -Wa,-mmsa,-mno-msa %s -Werror 2>&1 | FileCheck %s --check-prefix=CHECK-NOMMSA
// CHECK-NOMMSA: "-cc1"
// CHECK-NOMMSA-NOT: "-mssa"

// RUN: not %clang -### -c --target=x86_64 -Wa,-mmsa -Wa,-mno-msa %s 2>&1 | FileCheck %s --check-prefix=ERR
// ERR: error: unsupported argument '-mmsa' to option '-Wa,'
// ERR: error: unsupported argument '-mno-msa' to option '-Wa,'
Loading