[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for taking a look, @peixin! Just to clarify - I'm not really looking into `-Os`, `-Ofast` or `-Oz` at the moment. But I'm always happy to review driver patches :)



================
Comment at: clang/include/clang/Driver/Options.td:732
+def O_flag : Flag<["-"], "O">, Flags<[CC1Option,FC1Option]>, Alias<O>, AliasArgs<["1"]>;
 def Ofast : Joined<["-"], "Ofast">, Group<O_Group>, Flags<[CC1Option]>;
 def P : Flag<["-"], "P">, Flags<[CC1Option,FlangOption,FC1Option]>, Group<Preprocessor_Group>,
----------------
peixin wrote:
> Will enabling Ofast require more changes in the flang frontend? If yes, it is OK to support it in another patch later.
I would much prefer to have it implemented it in a dedicated patch.

For every option, one has to consider the semantics in the compiler driver (`flang-new`) vs frontend driver (`flang-new -fc1`) and then, in case of code-gen flags, the difference between middle-end and back-end pass pipelines. So quite a few things :)

In general, I'm in favor of doing this incrementally, in small patches. This makes reviewing and testing easier and more self-contained. Is that OK?


================
Comment at: flang/include/flang/Frontend/CodeGenOptions.def:30
+
+VALUE_CODEGENOPT(OptimizationLevel, 2, 0) ///< The -O[0-3] option specified.
+
----------------
peixin wrote:
> I saw `clang/include/clang/Basic/CodeGenOptions.def` has the following:
> ```
> VALUE_CODEGENOPT(OptimizationLevel, 2, 0) ///< The -O[0-3] option specified.
> VALUE_CODEGENOPT(OptimizeSize, 2, 0) ///< If -Os (==1) or -Oz (==2) is specified.
> ```
> 
> Do `-Os` and `-Oz` need extra processing in flang drivers? If yes, it is OK to support it in another patch later.
> Do -Os and -Oz need extra processing in flang drivers? 

These are code-size optimisation flags, so the logic will be a bit different.

Also, I'm very much in favor of small, incremental and self-contained patches. Splitting this into regular and size optimizations seemed like a natural split.


================
Comment at: flang/lib/Frontend/CodeGenOptions.cpp:8
+//===----------------------------------------------------------------------===//
+
+#include "flang/Frontend/CodeGenOptions.h"
----------------
peixin wrote:
> Miss the following?
> ```
> //
> // Coding style: https://mlir.llvm.org/getting_started/DeveloperGuide/
> //
> //===----------------------------------------------------------------------===//
> ```
> 
Good catch, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128043/new/

https://reviews.llvm.org/D128043