[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

Mon Sep 7 12:48:18 PDT 2020

awarzynski added a comment.

In D86089#2256139 <https://reviews.llvm.org/D86089#2256139>, @richard.barton.arm wrote:

> Another random thought that just came to me: what does the new driver do when you invoke it with no input files or options? I could imagine a few sensible outcomes (error: no input (clang and gcc/gfortran behaviour), print version and exit, print help and exit, etc.) and I don't have a strong preference over which we chose here but I think there should be a test for it in test/Driver

`flang-new` inherits generic things like this from libclangDriver. In this case:

  error: no input files

This is consistent with `clang`. I've added a test for this. As for `flang-new -fc1` - it in general does very little right now :) `clang -cc1` just sits there waiting

================
Comment at: flang/test/CMakeLists.txt:8

+llvm_canonicalize_cmake_booleans(FLANG_BUILD_NEW_DRIVER)
+
----------------
richard.barton.arm wrote:
> So do the other bools like LINK_WITH_FIR also need this treatment and this is a latent bug? Seems amazing we've not hit that before now.
> 
> From an offline conversation ISTR you saying this was to do with how the variable is translated into the lit config? If that is the case then I think there is a function you can use called lit.util.pythonize_bool which converts various strings that mean "True/False" into a real bool for python. That would also let you clean up the lit.cfg.py later on (separate comments).
AFAIK, `LINK_WITH_FIR` is not used in LIT tests.

I switched to `lit.util.pythonize_bool `, thanks for the suggestion!

================
Comment at: flang/test/Flang-Driver/driver-error-cc1.c:7
+
+// CHECK:error: unknown integrated tool '-cc1'. Valid tools include '-fc1'.
----------------
richard.barton.arm wrote:
> I am surprised that you don't need to prefix this run line with 'not' indicating that flang-new returns 0 exit code in this instance, which seems wrong to me.
Good catch, thanks! Fixed in the latest revision.

================
Comment at: flang/test/Flang-Driver/driver-error-cc1.cpp:1
+// RUN: %flang-new %s 2>&1 | FileCheck %s
+
----------------
richard.barton.arm wrote:
> Same comment as above on exit code.
Good catch, thanks! Fixed in the latest revision.

================
Comment at: flang/test/Flang-Driver/emit-obj.f90:2
+! RUN: %flang-new  %s 2>&1 | FileCheck %s --check-prefix=ERROR
+! RUN: %flang-new  -fc1 -emit-obj %s 2>&1 | FileCheck %s --check-prefix=ERROR
+
----------------
richard.barton.arm wrote:
> Seems like it would be helpful to have also implemented `-###` in this patch so that you don't need to write tests like this. You could simply run flang-new -### then check the -fc1 line that would have been emitted for the presence of -emit-obj.
> 
> Same comment above regarding exit code.
> Seems like it would be helpful to have also implemented -### in this patch

As`flang-new` is based on libclangDriver, we get `-###` for free (i.e. it's already supported).

However, there's a catch: https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Driver/Options.td#L369-L370. Currently `flang-new --help` will not display `-###`  because the the corresponding option definition hasn't been updated (i.e. it is not flagged as a Flang option):
```
def _HASH_HASH_HASH : Flag<["-"], "###">, Flags<[DriverOption, CoreOption]>,
    HelpText<"Print (but do not run) the commands to run for this compilation">;
``` 
We have three options:
1. Make `flang-new --help` display options that are flagged as `DriverOption` or `CoreOption`. Note this will include many other options (currently unsupported) and extra filtering would be required.
2. Add `FlangOption` to the definition of `_HASH_HASH_HASH`, but IMO that's a bit controversial. Is that what we want to do long-term? Or shall we create a separate category for generic driver options that apply to both Clang and Flang?
3. Delay the decision until there is more code to base our design on.

I'm in favor of Option 3.

================
Comment at: flang/test/lit.cfg.py:39
 # directories.
+# exclude the tests for flang_new driver while there are two drivers
 config.excludes = ['Inputs', 'CMakeLists.txt', 'README.txt', 'LICENSE.txt']
----------------
richard.barton.arm wrote:
> This comment should be on line 41
Updated, ta!

================
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@"
+
----------------
richard.barton.arm wrote:
> awarzynski wrote:
> > `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
> I think it might be better to use lit.util.pythonize_bool to do the canonicalisation. It would let you use config.include_flang_new_driver_test as a real bool later in the file rather than comparing to 1 or 0.
I did not know that one - thanks! Updated.

================
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:
> awarzynski wrote:
> > 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).
> This change is much better as it is more general and I think it could be acceptable.
> 
> I still don't get why you need to have anything set for FlagsToExclude in Flang's case? We only want to display flags that are specifically set as Flang options I think. So we set: 
> 
> FlagsToInclude=FlangOption
> FlagsToExclude=0
> 
> And all the options marked as both CC1Option, FlangOption are included because FlangOption is in the include list and all options without FlangOption are skipped. 
> 
> This will only display all the flags with FlangOption set. 
> 
> 
IIUC,  you're assuming that `FlagsToInclude != 0` implies that `FlagsToExclude == 0`.  However, once we have an option that's only relevant to Flang we'll start setting both `FlagsToInclude` and `FlagsToExclude` (i.e. that assumption will no longer hold).

But this change is needed _just yet_ and hence the confusion. Thanks for pointing out, I will keep it for later!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86089