[PATCH] D136080: [flang] Add -ffp-contract option processing

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for implementing this, @tblah!

Two high level questions/requests:

- are you confident that we will need LangOptions.def?
- can you upload a patch with full context? (some details can be found here: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)

-Andrzej



================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:85-86
+                                    ArgStringList &CmdArgs) {
+  // TODO: share RenderFloatingPointOptions from ./Clang.cpp and use that
+  // instead of duplicating code here
+  StringRef FPContract;
----------------
What's RenderFloatingPointOptions?


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:664
+///
+/// \param [out] res Stores the processed arguments
+/// \param [in] args The arguments to parse
----------------
`res` is a very confusing name (that I used myself in various places). Basically, it's the `CompilerInvocation` instance ... result. Perhaps use `invoc`? 


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:665
+/// \param [out] res Stores the processed arguments
+/// \param [in] args The arguments to parse
+/// \param [out] diags DiagnosticsEngine to report erros with
----------------
[nit] "The compiler invocation args to parse"


================
Comment at: flang/test/Driver/driver-help.f90:108
 ! HELP-FC1-NEXT: Use <value> as character line width in fixed mode
+! HELP-FC1-NEXT: -ffp-contract=<value> Form fused FP ops (e.g. FMAs): fast (fuses across statements disregarding pragmas) | on (only fuses in the same statement unless dictated by pragmas) | off (never fuses) | fast-honor-pragmas (fuses across statements unless diectated by pragmas). Default is 'fast' for CUDA, 'fast-honor-pragmas' for HIP, and 'on' otherwise.
 ! HELP-FC1-NEXT: -ffree-form            Process source files in free form
----------------
Why not expose this flag in `flang-new`? (as well as `flang-new -fc1`?)


================
Comment at: flang/test/Driver/flang_fp_opts.f90:1-2
+! Test for passing of floating point options between the compiler and frontend
+! drivers.
+
----------------
Sounds like a test for [[ https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/frontend-forwarding.f90 | frontend-forwarding.f90 ]]


================
Comment at: flang/test/Driver/flang_fp_opts.f90:6-7
+
+! CHECK1-LABEL: "-fc1"
+! CHECK1: -ffp-contract=fast
+
----------------
Could you use more descriptive prefixes?


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

https://reviews.llvm.org/D136080