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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

> The omission of the fast-honor-pragmas argument from the compiler driver is deliberate.

Where is it omitted?

> I suspect the CI failure on windows is unrelated to my code

I agree.



================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:165
+  // Floating point related options
+  AddFloatingPointOptions(D, Args, CmdArgs);
+
----------------
>From [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | LLVM Coding style ]]

> Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo()).

I know that this style is not followed here 100%. But that's what we should aim for :)


================
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;
----------------
tblah wrote:
> awarzynski wrote:
> > What's RenderFloatingPointOptions?
> It is a static function in clang/lib/Driver/ToolChains/Clang.cpp which does the same job as AddFloatingPointOptions, except for clang. I couldn't use it right away because not all of the options it it processes are supported in flang, but once we get there it would make sense to share code.
Ah! That [[ https://github.com/llvm/llvm-project/blob/b1d7a95e4e4a2b57cbe02636bbe357dc48d615c5/clang/lib/Driver/ToolChains/Clang.cpp#L2753-L3185 | function ]] is over 400 LOC and looks super complex. I doubt Flang will be able to share that logic with Clang any time soon. If ever. I would actually skip this comment. Sometimes code duplication is the right approach ;-)


================
Comment at: flang/include/flang/Frontend/LangOptions.h:9-11
+//  This file defines the CodeGenOptions interface, which holds the
+//  configuration for LLVM's middle-end and back-end. It controls LLVM's code
+//  generation into assembly or machine code.
----------------
UPDATEME


================
Comment at: flang/include/flang/Frontend/LangOptions.h:29
+
+    // Enable the floating point pragma
+    FPM_On,
----------------
What are these pragmas? Perhaps you can add a test that would include them?


================
Comment at: flang/include/flang/Frontend/LangOptions.h:49-50
+
+/// Tracks various options which control the dialect of fortran that is
+/// accepted. Based on clang::LangOptions
+class LangOptions : public LangOptionsBase {
----------------



================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:661-662
 
+/// Parses all floating point related arguments and populates the variables
+/// options accordingly. Returns false if new errors are generated.
+///
----------------
> populates the variables options accordingly

Perhaps this would be a bit more specific/descriptive:

"and configures the input CompilerInvocation accordingly". Ultimately, this is about ... configuring the current compiler invocation, which is represented by an instance of `CompilerInvocaiton`. 


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:693
+
+    // TODO: actually use the flag in codegen
+    diags.Report(diagUnimplemented) << a->getOption().getName();
----------------
In here you only configure `CompilerInvocation`. This configuration will be used elsewhere (i.e. where codegen happens). So, I think, as far as this method is concerned the implementation is complete.


================
Comment at: flang/test/Driver/flang_fp_opts.f90:1-2
+! Test for passing of floating point options between the compiler and frontend
+! drivers.
+
----------------
No longer valid ;-)


================
Comment at: flang/test/Driver/flang_fp_opts.f90:4
+
+! RUN: %flang_fc1 -ffp-contract=fast %s 2>&1 | FileCheck %s
+! CHECK: ffp-contract= is not currently implemented
----------------
Can you test with `flang` as well?


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

https://reviews.llvm.org/D136080