[flang][driver] add negative from of -fsave-main-program

[jeanPerier]

Add the -fno form for consistency and to make it easy to switch the default for downstream users.

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-flang-fir-hlfir
@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-flang-driver

Author: None (jeanPerier)

Changes

Add the -fno form for consistency and to make it easy to switch the default for downstream users.

---

Full diff: https://github.com/llvm/llvm-project/pull/124110.diff

5 Files Affected:

• (modified) clang/include/clang/Driver/Options.td (+2-2)
• (modified) clang/lib/Driver/ToolChains/Flang.cpp (+2-1)
• (modified) flang/lib/Frontend/CompilerInvocation.cpp (+5-4)
• (modified) flang/test/Driver/fsave-main-program.f90 (+5-1)
• (modified) flang/test/Lower/fsave-main-program.f90 (+1)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index df705104d9ea31..6fface303c57a6 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -6968,8 +6968,8 @@ defm unsigned : OptInFC1FFlag<"unsigned", "Enables UNSIGNED type">;
 def fno_automatic : Flag<["-"], "fno-automatic">, Group<f_Group>,
   HelpText<"Implies the SAVE attribute for non-automatic local objects in subprograms unless RECURSIVE">;
 
-def fsave_main_program : Flag<["-"], "fsave-main-program">, Group<f_Group>,
-  HelpText<"Place all variables from the main program in static memory (otherwise scalars may be placed on the stack)">;
+defm save_main_program : OptInFC1FFlag<"save-main-program",
+  "Place all variables from the main program in static memory (otherwise scalars may be placed on the stack)">;
 
 defm stack_arrays : BoolOptionWithoutMarshalling<"f", "stack-arrays",
   PosFlag<SetTrue, [], [ClangOption], "Attempt to allocate array temporaries on the stack, no matter their size">,
diff --git a/clang/lib/Driver/ToolChains/Flang.cpp b/clang/lib/Driver/ToolChains/Flang.cpp
index 9c1fd28a3a8a26..8d1ec016325dfb 100644
--- a/clang/lib/Driver/ToolChains/Flang.cpp
+++ b/clang/lib/Driver/ToolChains/Flang.cpp
@@ -58,7 +58,8 @@ void Flang::addFortranDialectOptions(const ArgList &Args,
                             options::OPT_fhermetic_module_files,
                             options::OPT_frealloc_lhs,
                             options::OPT_fno_realloc_lhs,
-                            options::OPT_fsave_main_program});
+                            options::OPT_fsave_main_program,
+                            options::OPT_fno_save_main_program});
 }
 
 void Flang::addPreprocessingOptions(const ArgList &Args,
diff --git a/flang/lib/Frontend/CompilerInvocation.cpp b/flang/lib/Frontend/CompilerInvocation.cpp
index 3c6da4687f65d3..68b5950d3a51b7 100644
--- a/flang/lib/Frontend/CompilerInvocation.cpp
+++ b/flang/lib/Frontend/CompilerInvocation.cpp
@@ -770,10 +770,11 @@ static bool parseFrontendArgs(FrontendOptions &opts, llvm::opt::ArgList &args,
     opts.features.Enable(Fortran::common::LanguageFeature::DefaultSave);
   }
 
-  // -fsave-main-program
-  if (args.hasArg(clang::driver::options::OPT_fsave_main_program)) {
-    opts.features.Enable(Fortran::common::LanguageFeature::SaveMainProgram);
-  }
+  // -f{no}-save-main-program
+  opts.features.Enable(
+      Fortran::common::LanguageFeature::SaveMainProgram,
+      args.hasFlag(clang::driver::options::OPT_fsave_main_program,
+                   clang::driver::options::OPT_fno_save_main_program, false));
 
   if (args.hasArg(
           clang::driver::options::OPT_falternative_parameter_statement)) {
diff --git a/flang/test/Driver/fsave-main-program.f90 b/flang/test/Driver/fsave-main-program.f90
index bffdfd97911e80..e7a2f9d8b470ed 100644
--- a/flang/test/Driver/fsave-main-program.f90
+++ b/flang/test/Driver/fsave-main-program.f90
@@ -1,5 +1,9 @@
 ! Check that the driver passes through -fsave-main-program:
 ! RUN: %flang -### -S -fsave-main-program %s -o - 2>&1 | FileCheck %s
+! CHECK: "-fc1"{{.*}}"-fsave-main-program"
+
+! RUN: %flang -### -S -fno-save-main-program %s -o - 2>&1 | FileCheck %s --check-prefix=CHECK2
+! CHECK2: "-fc1"{{.*}}"-fno-save-main-program"
+
 ! Check that the compiler accepts -fsave-main-program:
 ! RUN: %flang_fc1 -emit-hlfir -fsave-main-program %s -o -
-! CHECK: "-fc1"{{.*}}"-fsave-main-program"
diff --git a/flang/test/Lower/fsave-main-program.f90 b/flang/test/Lower/fsave-main-program.f90
index 17fc1b02f5068f..e89244c3c7c51a 100644
--- a/flang/test/Lower/fsave-main-program.f90
+++ b/flang/test/Lower/fsave-main-program.f90
@@ -1,6 +1,7 @@
 ! Test -fsave-main-program switch.
 ! RUN: %flang_fc1 -emit-hlfir -o - %s | FileCheck --check-prefix=CHECK-DEFAULT %s
 ! RUN: %flang_fc1 -fsave-main-program -emit-hlfir -o - %s | FileCheck --check-prefix=CHECK-SAVE %s
+! RUN: %flang_fc1 -fsave-main-program -fno-save-main-program -emit-hlfir -o - %s | FileCheck --check-prefix=CHECK-DEFAULT %s
 program test
 integer :: i
 call foo(i)

[kiranchandramohan]

LG. Thanks Jean.

Do you need a bbc option?

[eugeneepshteyn]

Question on OptInFC1FFlag usage. It's definition:

multiclass OptInFC1FFlag<string name, string pos_prefix, string neg_prefix="",
                         string help="",
                         list<OptionVisibility> vis=[ClangOption]> {

So with the current usage, doesn't it assign "save-main-program" to name and then the help text to pos_prefix?

[jeanPerier]

It does. I just copied from other usages, but looking into it, I think that is the intention for this from.

The prefixes are for the help messages and only the positive from of the flag is printed in the help since the negative is the default behavior (hence the "OptIn"). So this sets the help message of the positive form via pos_prefix and leaves the negative from help text empty (neg_prefix # help)).

But I actually want the negative from to be printed, I am not a fan of hidden options, so I changed to BoolOptionWithoutMarshalling. Thanks for raising the question!

[jeanPerier]

LG. Thanks Jean.

Do you need a bbc option?

Thanks for the review @kiranchandramohan!

I would go for no. bbc does not even has a proper -fno-automatic flag. I do not see it as a goal for bbc to define all the options flang has, its goal is mainly to remain simple for easy debugging of the lowering flow.

[tarunprabhu]

Why is this a ClangOption?

I am not saying that it is wrong, but it seems odd. I do see that the same has been done for the ppc_native_vec_elem_order option above also. What happens if the flag is passed to clang instead of flang?

[jeanPerier]

Thanks for pointing that out. I indeed took that from the example above. The flag is not accepted by clang, but this ClangOption is just misleading and useless: it is actually overridden because this definition is made in a let Visibility = [FC1Option, FlangOption] in { scope.

I removed it here and will clean-up the other flags in a separate patch.

[tarunprabhu]

Thanks for the clarification, Jean.

[tarunprabhu]

LGTM. Thanks!