[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen
Andrzej Warzynski via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 26 09:20:26 PDT 2020
awarzynski added a comment.
@CarolineConcatto - thank you for working on this! @all, thank you for your reviews, this is much appreciated!
While Carol is away, I'll try my best to address all the outstanding comments. I will also update the patch accordingly. I've identified a few small issues myself - that will also be updated in the next patch.
================
Comment at: clang/lib/Driver/Driver.cpp:1573
+ ExcludedFlagsBitmask |= options::CC1Option;
+ IncludedFlagsBitmask |= options::FlangOption;
+ } else
----------------
sameeranjoshi wrote:
> In `enum ClangFlags`
> inside File `clang/include/clang/Driver/Options.h`
> there are various other options do they need to be considered?
> If not, how are they handled?
>
> do they need to be considered?
Not at this stage. This is sufficient to make sure that `flang-new -help` only prints options relevant to Flang.
We may want to fine-tune this in the future, but currently it would be a bit tricky with only 2 options being supported ;-)
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:11
+
+#include "flang/Frontend/CompilerInvocation.h"
+
----------------
CarolineConcatto wrote:
> tskeith wrote:
> > Why is this called "Frontend" rather than "Driver"?
> The way we think is that the driver is split in:
> **the driver** libDriver -> that implements the driver. ATM it is inside clang/lib/Driver. And it can be clang( called inside clang/tools/driver/driver.cpp) or flang-new(called inside flang/tools/driver/driver.cpp) according to the mode the driver is called.
> **the frontend driver** -> it can be:
> clang frontend driver: clang -cc1 that calls libclangfrontend
> flang frontend driver: flang-new -fc1 that calls libflangfrontend.
> the -xc1 has its functions/methods implemented inside the driver Frontend
>
> So this folder is called Frontend because it belongs to the part that implements the driver frontend of the compiler and we would like to leave this way so it has the same design as clang.
>
Just to further clarify, I think that it's important to distinguish between:
* the compiler driver, `flang`, implemented in terms of `libclandDriver` (it creates and executes jobs/commands, e.g. linker or frontend jobs)
* the frontend driver, `flang -fc1`, implemented in terms of `libflangFrontend` (it creates frontend actions, e.g. preprocessor actions)
"Frontend" in this case means "Frontend Driver". "Driver" is reserved for the compiler driver, i.e. "libclangDriver".
As Carol mentioned, our terminology is inspired by Clang. Keeping the names consistent felt like a good idea. @tskeith, would you prefer "FrontendDriver" instead of "Frontend"?
(`libclangDriver` is the Clang compiler driver library that we want to re-use, `libflangFrontend` is the Flang frontend driver library, inspired by `libclangFrontend`)
================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:31-32
+ Language lang_;
+ unsigned fmt_ : 3;
+ unsigned preprocessed_ : 1;
+
----------------
> Why isn't the type bool?
@tskeith Bitfields are used here for optimization. An instance of `InputKind` is created per input file, so this could be a substantial saving (if the bitfields happen to be laid out nicely).
Btw, we won't need `fmt_` or `preprocessed_` (and the corresponding member methods) just now. I will delete them.
================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:65
+ /// Show the -version text.
+ unsigned ShowVersion : 1;
+
----------------
CarolineConcatto wrote:
> tskeith wrote:
> > Why aren't the types `bool`?
> Most of these things are going to be set by option::driver. That is the reason it is as it is.
> Why aren't the types bool?
@tskeith See above.
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:46
+ InputKind DashX(Language::Unknown);
+ return DashX;
+ }
----------------
CarolineConcatto wrote:
> tskeith wrote:
> > Why not `return InputKind{Language::Unknown};`?
> Just because the next lines also return DashX.
I agree with @tskeith , `return InputKind{Language::Unknown};` would be cleaner. I will update this.
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:91
+ InputKind DashX = ParseFrontendArgs(Res.getFrontendOpts(), Args, Diags);
+ (void)DashX;
+
----------------
tskeith wrote:
> What is the purpose of `DashX` here?
Not needed for now, I will delete it. Thanks!
================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:20
+
+using namespace flang;
+namespace flang {
----------------
CarolineConcatto wrote:
> tskeith wrote:
> > What is this for?
> Because of CompilerInstance *Flang.
It's not needed, good catch, cheers!
================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:39
+
+ return true;
+}
----------------
sameeranjoshi wrote:
> The comment in header for `ExecuteCompilerInvocation` mentions,
> ```
> /// \return - True on success.
> bool ExecuteCompilerInvocation(CompilerInstance *Flang);
> ```
>
> Do we need to have a `false` somewhere here?
>
> I see 2 scenarios when `ExecuteCompilerInvocation` might fail (there could definitely be more) and there we need an indication of failure by returning `false`,
> 1. When there is no actual execution of compiler.
> 2. The compilation in not successful.
>
>
@sameeranjoshi These are good points, thank you! However, currently `ExecuteCompilerInvocation` doesn't do much. When this patch is approved and commited, we'll start adding actual _actions_ (to be executed by this method) that _could_ fail. At that point we will introduce code paths that could lead to `return false`.
================
Comment at: flang/test/lit.cfg.py:40
+# exclude the tests for flang_new driver while there are two drivers
+if config.include_flang_new_driver_test == "OFF":
+ config.excludes = ['Inputs', 'CMakeLists.txt', 'README.txt', 'LICENSE.txt', 'Flang-Driver']
----------------
CarolineConcatto wrote:
> richard.barton.arm wrote:
> > richard.barton.arm wrote:
> > > I think it would be cleaner to define config.excludes unconditionally then append the Flang-Driver dir if our condition passes.
> > I _think_ the pattern to follow to exclude tests for something you haven't built is to use lit features.
> >
> > So you would add a feature like:
> > `config.available_features.add("new-driver")`
> >
> > then each test that only works on the new driver would be prefixed with a statement:
> >
> > `REQUIRES: new-driver`
> >
> > This means that the tests can go in the test/Driver directory and you don't need to create a new directory for these tests or this hack. The additional benefit would be that all the existing tests for the throwaway driver can be re-used on the new Driver to test it. There are not many of those though and we are using a different driver name so they can't be shared either. Still think it would be a worthwhile thing to do because when looking at the test itself it is clear why it is not being run whereas with this hack it is hidden away.
> >
> > Sorry for not thinking this first time around.
> I like to have the test at a different folder for now so it is clear that the tests inside this folder all belongs to the new driver. So I don't need to open the test to know.
> I can implement the requires, but in the future will not need the REQUIRES for the driver test.
Let me clarify a bit. I assume that `FLANG_BUILD_NEW_DRIVER` is Off.
SCENARIO 1: In order to make sure that `bin/llvm-lit tools/flang/test/` does not _attempt to run_ the new driver tests, add:
`config.excludes.append('Flang-Driver')`
With this, the new driver tests will neither be run nor summarized (e.g. as `UNSUPPORTED`) in the final output.
SCENARIO 2: `config.excludes.append('Flang-Driver')` will not affect `bin/llvm-lit tools/flang/test/Flang-Driver` (this time I explicitly specify the `Flang-Driver` directory). We need:
`REQUIERES: new-flang-driver`
(or similar) for the new Flang driver tests to be reported as `UNSUPPORTED`.
I agree with @richard.barton.arm that `REQUIRES` would be the more canonical approach. We could still leave `config.excludes.append('Flang-Driver')` there to reduce the noise when running `bin/llvm-lit tools/flang/test/`. Unless people object, I recommend that we have both.
================
Comment at: flang/test/lit.site.cfg.py.in:14
+# control the regression test for flang-new driver
+config.include_flang_new_driver_test="@FLANG_BUILD_NEW_DRIVER@"
+
----------------
`FLANG_BUILD_NEW_DRIVER` should be canonicalized in CMake first: https://github.com/llvm/llvm-project/blob/c79a366ec0f87eafca123138b550b039618bf880/llvm/cmake/modules/AddLLVM.cmake#L1424-L1435
================
Comment at: flang/tools/flang-driver/driver.cpp:30
+extern int fc1_main(
+ llvm::ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr);
+
----------------
tskeith wrote:
> Why isn't this declared in a header?
We'd be creating a dedicated header file for no real benefit (this method should not be re-used anywhere else).
================
Comment at: flang/tools/flang-driver/fc1_main.cpp:1
+//===-- fc1_main.cpp - Flang FC1 Compiler Frontend ------------------------===//
+//
----------------
SouraVX wrote:
> This file-name seems odd considering LLVM style. How about just `Flang.cpp` or `FlangMain.cpp` ? or other
This is for consistency with `clang`: https://github.com/llvm/llvm-project/tree/master/clang/tools/driver. Perhaps `Fc1Main.cpp` instead?
================
Comment at: flang/tools/flang-driver/fc1_main.cpp:54
+ // Execute the frontend actions.
+ { Success = ExecuteCompilerInvocation(Flang.get()); }
+
----------------
tskeith wrote:
> Why is this statement in a block? Is the result of CreateFromArgs supposed to affect the return value of this function?
Copy and paste error - cheers for pointing out, fixed in the next revision!
================
Comment at: llvm/lib/Option/OptTable.cpp:621
+ // If `Flags` is empty (i.e. it's an option without any flags) then this is
+ // a Clang-only option. If:
+ // * we _are not_ in Flang Mode (FlagsToExclude contains FlangMode), then
----------------
richard.barton.arm wrote:
> This is not the sort of change I expect to see in an llvm support library given that it seems very clang and flang specific.
>
> I think this needs to be re-written to be more generic, perhaps tweaking the interface to be able to express the logic you want to use for flang and clang.
>
> Why is it not sufficient to call this function unchanged from both flang and clang but with a different set of FlagsToInclude/FlagsToExclude passed in using this logic to set that on the clang/flang side?
I agree and have updated this. Thanks for pointing it out!
This part is particularly tricky for Flang. Flang has options with flags that fall both into `FlagsToExclude` and into `FlagsToInclude`. For example:
```
def help : Flag<["-", "--"], "help">, Flags<[CC1Option,CC1AsOption,FlangOption]>,
HelpText<"Display available options">;
```
For Flang, before this method is called,
* `CC1Option` is added to `FlagsToExclude`, and
* `FlangOption` is added to `FlagsToInclude`
AFAIK, this is unique for Flang. Other tools never set both `FlagsToExclude` and `FlagsToInclude`. As a result, the currently vague relation between `FlagsToExclude` and `FlagsToInclude` needs to be clarified. In my opinion, `FlagsToInclude` should take precedence over `FlagsToExclude`. That's what I've implemented (that's clarified in the next revision).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86089/new/
https://reviews.llvm.org/D86089
More information about the llvm-commits
mailing list