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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
