-
Notifications
You must be signed in to change notification settings - Fork 0
Add options -m[no-]zos-ppa1-name to remove the function name in PPA1 on z/OS. #2
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.
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
Changes from 4 commits
640ce91
32b8815
b87a9a4
6c45a04
78432c4
7e1a714
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3809,6 +3809,11 @@ void CompilerInvocationBase::GenerateLangArgs(const LangOptions &Opts, | |
| if (Opts.IgnoreXCOFFVisibility) | ||
| GenerateArg(Consumer, OPT_mignore_xcoff_visibility); | ||
|
|
||
| if (Opts.getZOSPPA1Name() == LangOptions::ZOSPPA1NameKind::Emit) | ||
| GenerateArg(Consumer, OPT_mzos_ppa1_name); | ||
| else if (Opts.getZOSPPA1Name() == LangOptions::ZOSPPA1NameKind::NoEmit) | ||
| GenerateArg(Consumer, OPT_mno_zos_ppa1_name); | ||
|
|
||
| if (Opts.SignedOverflowBehavior == LangOptions::SOB_Trapping) { | ||
| GenerateArg(Consumer, OPT_ftrapv); | ||
| GenerateArg(Consumer, OPT_ftrapv_handler, Opts.OverflowHandler); | ||
|
|
@@ -4227,6 +4232,16 @@ bool CompilerInvocation::ParseLangArgs(LangOptions &Opts, ArgList &Args, | |
| if (T.isOSAIX() && (Args.hasArg(OPT_mignore_xcoff_visibility))) | ||
| Opts.IgnoreXCOFFVisibility = 1; | ||
|
|
||
| if (const Arg *A = Args.getLastArg(OPT_mzos_ppa1_name, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow the pattern AIX has above. If it is z/OS query the option and set the Opts. You don't need to handle the unsupported error if you follow that pattern. If
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What mechanism guarantees this in clang cc1? For the above AIX option, I tried this The definition of this option in clang/include/clang/Options/Options.td is It is handled in clang/lib/Driver/ToolChains/AIX.cpp I think Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Don't worry about generating an error from -cc1 for not z/OS systems. Just make sure you only check for the option if the target is z/OS. That will leave the option as unclaimed for non z/OS systems.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree. So
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. This happens only in clang Driver, but not in cc1.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I am emitting an error from cc1 because most downstream zos-only options do this.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I am not sure whether you are mixing clang Driver and cc1.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I have made this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That may be the case, but it's good to always follow the practice of checking the context before checking the option. Use "if zos and hasArg" vs "if hasArg and zos". |
||
| OPT_mno_zos_ppa1_name)) { | ||
| if (T.isOSzOS()) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use following as it's faster, follows the usual convention and avoids marking an option as claimed when it isn't used. if (T.isOSzOS()) {
if (const Arg *A = Args.getLastArg(OPT_mzos_ppa1_name, OPT_mno_zos_ppa1_name)) {
...
}
}
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! I will make the change. Thanks! |
||
| if (A->getOption().matches(OPT_mzos_ppa1_name)) | ||
| Opts.setZOSPPA1Name(LangOptions::ZOSPPA1NameKind::Emit); | ||
| else | ||
| Opts.setZOSPPA1Name(LangOptions::ZOSPPA1NameKind::NoEmit); | ||
| } | ||
| } | ||
|
|
||
| if (Args.hasArg(OPT_ftrapv)) { | ||
| Opts.setSignedOverflowBehavior(LangOptions::SOB_Trapping); | ||
| // Set the handler, if one is specified. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| // REQUIRES: systemz-registered-target | ||
|
|
||
| // test default | ||
|
sujianIBM marked this conversation as resolved.
|
||
| // RUN: %clang_cc1 -triple s390x-ibm-zos -emit-llvm %s -o -\ | ||
| // RUN: | FileCheck %s -check-prefix=DEFAULT | ||
|
|
||
| // test the positive and negative options | ||
| // RUN: %clang_cc1 -triple s390x-ibm-zos -mzos-ppa1-name -emit-llvm %s -o -\ | ||
| // RUN: | FileCheck %s -check-prefix=EMIT-NAME | ||
| // RUN: %clang_cc1 -triple s390x-ibm-zos -mno-zos-ppa1-name -emit-llvm %s -o -\ | ||
| // RUN: | FileCheck %s -check-prefix=NOT-EMIT-NAME | ||
|
|
||
| // DEFAULT-NOT: attributes #0 = {{{.*}}"zos-ppa1-name"{{.*}}} | ||
| // EMIT-NAME: attributes #0 = {{{.*}}"zos-ppa1-name"="true"{{.*}}} | ||
| // NOT-EMIT-NAME: attributes #0 = {{{.*}}"zos-ppa1-name"="false"{{.*}}} | ||
|
|
||
| int main() { | ||
| return 0; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // REQUIRES: systemz-registered-target | ||
|
|
||
| // RUN: %clang -### -target s390x-ibm-zos -mzos-ppa1-name -c %s 2>&1 \ | ||
| // RUN: | FileCheck -check-prefix=EMIT-NAME %s | ||
| // EMIT-NAME: "-mzos-ppa1-name" | ||
|
|
||
| // RUN: %clang -### -target s390x-ibm-zos -mno-zos-ppa1-name -c %s 2>&1 \ | ||
| // RUN: | FileCheck -check-prefix=NOT-EMIT-NAME %s | ||
| // NOT-EMIT-NAME: "-mno-zos-ppa1-name" | ||
|
|
||
| // RUN: not %clang -target systemz-unknown-elf -mzos-ppa1-name -c %s 2>&1 \ | ||
| // RUN: | FileCheck -check-prefix=ERR %s | ||
| // RUN: not %clang -target systemz-unknown-elf -mno-zos-ppa1-name -c %s 2>&1 \ | ||
| // RUN: | FileCheck -check-prefix=ERR %s | ||
| // ERR: error: unsupported option '-m{{.*}}zos-ppa1-name' for target 'systemz-unknown-elf' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a string, I'd use values more descriptive like "include"/"exclude". That will leave room for other values (eg. leaf or not). If it's a value can't you use true/false constants?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a string with a value.
My understanding (mainly from copilot)
LLVM supports 3 main kinds:
I am using the 2nd one.
The first one is over-killing in our case.
The 3rd one, I think the underneath type is int.
I will make the values more descriptive. Is it ok to use "emit" and "no-emit"? They match with LangOpts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'm fine with different values as long as those values that make the code in llvm that use them self documenting. A names like "always" and "none" could be better.
The llvm code never uses the false value. It would be good to add an assert to make sure the value is one of the two set here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean
"zos-ppa1-name"=["always"|"none"]?I actually feel they are quite confusing.
To start with, they are not a pair. "always" should be grouped with "sometimes" and "never", and "none" should be grouped with "single" and "multiple".
I don't think "emit" and "no-emit" are perfect, but they match with the LangOpts option. I'd like to use them if you don't object.
I don't follow very well.
Do you mean to add an assert of
ZOSPPA1Name = M.getLangOpts().getZOSPPA1Name()only havingclang::LangOptions::ZOSPPA1NameKind::Emitandclang::LangOptions::ZOSPPA1NameKind::NoEmithere in clang/lib/CodeGen/Targets/SystemZ.cpp?I think the values
ZOSPPA1Namecan have are limited byenum class ZOSPPA1NameKind.Or you want to add an assert in
SystemZAsmPrinter::calculatePPA1()that the "zos-ppa1-name" attribute only has two values, in case this attribute is set somewhere else by mistake?Thanks!