richard.barton.arm requested changes to this revision.
richard.barton.arm added a comment.
This revision now requires changes to proceed.
Requesting changes mostly because of the exit status issue on the Driver tests.
A few general questions as well:
1. Why not implement `-###` as part of this patch to enable testing more easily?
2. How come there are no regression tests for -fc1 in flang/test/Frontend? I
suppose these come when the first real FrontendAction is implemented?
================
Comment at: flang/test/CMakeLists.txt:8
+llvm_canonicalize_cmake_booleans(FLANG_BUILD_NEW_DRIVER)
+
----------------
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).
================
Comment at: flang/test/Flang-Driver/driver-error-cc1.c:7
+
+// CHECK:error: unknown integrated tool '-cc1'. Valid tools include '-fc1'.
----------------
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.
================
Comment at: flang/test/Flang-Driver/driver-error-cc1.cpp:1
+// RUN: %flang-new %s 2>&1 | FileCheck %s
+
----------------
Same comment as above on exit code.
================
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
+
----------------
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.
================
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']
----------------
This comment should be on line 41
================
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']
----------------
awarzynski wrote:
> 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.
Happy with this approach :-)
================
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@"
+
----------------
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.
================
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
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86089/new/
https://reviews.llvm.org/D86089
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
