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

Wed Jun 22 10:22:54 PDT 2022

awarzynski added inline comments.

================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:133
+      CmdArgs.push_back("-O3");
+      TC.getDriver().Diag(diag::warn_O4_is_O3);
+    } else {
----------------
peixin wrote:
> Nit: I have committed D126164, and you can rebase and use D directly, which is the style in `Clang.cpp`.
Thanks for the heads up and for seeing https://reviews.llvm.org/D126164 through! I'll update it now before I forget :)

================
Comment at: flang/include/flang/Frontend/CodeGenOptions.def:12
+// Optionally, the user may also define ENUM_CODEGENOPT (for options
+// that have enumeration type and VALUE_CODEGENOPT is a code
+// generation option that describes a value rather than a flag.
----------------
rovka wrote:
> I'm not sure I understand the difference between CODEGENOPT and VALUE_CODEGENOPT. Is it that CODEGENOPT is actually a kind of BOOL_CODEGENOPT, that should always have just 1 bit? Do we really need both?
At one point I convinved myself that I understand the difference, but now I'm re-rereading Clang's [[ https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/CodeGenOptions.def | CodeGenOptions.def ]] and I also see ... no difference 😂  .

Thanks for pointing this out! Let me remove it.

================
Comment at: flang/include/flang/Frontend/CodeGenOptions.def:25
+
+#ifndef ENUM_CODEGENOPT
+#  define ENUM_CODEGENOPT(Name, Type, Bits, Default) \
----------------
rovka wrote:
> This isn't used yet, can we skip it?
Yup, good suggestion!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128043