[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added inline comments.



Comment at: clang/lib/Driver/ToolChain.cpp:185
   {"flang", "--driver-mode=flang"},
+  {"flang-new", "--driver-mode=flang"},
   {"clang-dxc", "--driver-mode=dxc"},

clementval wrote:
> This is counter intuitive. We rename flang-new but we add flang-new here. It 
> should be configurable. 
This is a list of all the valid prefixes of binaries that the driver supports. 
With this change, an additional one will be supported so I don't think it's an 
issue to have both flang and flang-new here.

The thing that confuses me is how flang-new works today without this change, 
given that "flang" is not a suffix of "flang-new"? @awarzynski , do you know 
why that is? Perhaps the line here is not needed at all? Or is this a bug fix 
for flang-new that is actually unrelated to this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122008: [flang][driver] Add support for generating executables

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.

Option 1 for me, let's not delay.

With upstreaming of fir-dev making great progress, we can start to think about 
having a working flang in LLVM 15. For that we would need to have upstreaming 
finished, the driver finished and CMake support finished before end of June. 
The initial user experience work should be focused on LLVM 15. The sooner we 
have the code on main branch, the sooner we can begin the work of shaking out 
the problems. To delay means that flang in LLVM 15 will not be as good as it 
could be.

This patch is orthogonal to the issues discussed on this thread and on the call 
on 4th April. Anyone using flang today is going to see those issues just the 
same with or without this patch as flang will error out before it gets to 
linking. Committing this patch won't change what flang can't do yet but it will 
probably flush out some more issues - perhaps serious ones - for us to fix. I 
don't think we should be scared to receive bug reports. At this stage in 
development, bug reports are surely good for us and the sooner we get them, the 
sooner we can potentially fix the issues.

I think the group of interested users with high quality-level expectations for 
flang are going to be evaluating it on a release rather than at some random 
point on main. I think anyone following along on main is going to have an 
accurate understanding of flang's maturity there and unlikely to be surprised 
by the quality of it. We should consider asking Alex to skip this patch in 
LLVMWeekly, or ask him to message it as we want to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122008

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97119: [flang][driver] Add options for -std=f2018

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.

I agree with @tskeith that `-Mstandard` is not exactly orthogonal to `-std`, 
the former being about warning for non-standard extensions/deviations and the 
latter being about use of standard fortran, but from a different standard 
version. I would expect `-Mstandard` to apply to whatever value of `-std` you 
gave the compiler.

I think the gfortran analog to `-Mstandard` is `-fpedantic` or 
`-fpedantic-errors` - see 
https://gcc.gnu.org/onlinedocs/gfortran/Error-and-Warning-Options.html#Error-and-Warning-Options.
 The docs have a little joke in them about the feature not doing what users 
want, but I think that's just a witty way to say that the feature is 
incomplete, but that it is expected to catch things in theory. So maybe 
`-fpedantic{-errors}` is a good thing to substitute for `-Mstandard`?

FWIW, I think supporting `-std` with only one supported option is a reasonable 
thing to do and will help with folks porting codes to flang from gfortran to 
accept the option and to know what's supported and what is not. Perhaps the 
perfect thing would be supporting all the sensible gfortran values for `-std`, 
but emitting a (downgradable) warning or remark when one that is not supported 
is given. That would let users know what they are dealing with until the day 
when flang supports other values for `-std`. I doubt supporting older versions 
is on anyone's short-term roadmap, but patches would probably be welcomed on 
that. I guess there will one day be a `-std=f202X` option supported too!


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

https://reviews.llvm.org/D97119

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94169: [clang][driver] Restore the original help text for `-I`

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.

I think the right long-term solution is going to be SUGGESTION 2, or a variant 
of that. We have implemented this feature downstream in Arm Compiler for Linux, 
i.e. a separate, normally longer and more detailed, Doc string for the 
CommandLineReference.rst file from the --help string. Such a mechanism could 
perhaps be extended to add Clang- and/or Flang- specific doc strings or 
additional doc strings. That's a big piece of work, but very worthy IMO.

To unblock development today I would support Andrzej's SUGGESTION 1. Perhaps 
saying "For C++ input ..." to distinguish the Clang-only behaviour for now.

In writing the above I double checked the Options.td to see if anything 
upstream exists today. There is a field DocBrief that appears to be preferred 
over HelpText in the ClangCommandLineReference.rst. Perhaps the new, more 
detailed but C++ specific message could be a DocBrief field, so for the 
ClangCommandLineReference.rst only and we can revert to the original shorter 
HelpText that applies for both C++ and Fortran? Perhaps that would actually be 
a better short-term compromise in that it meets @andreil99 's needs as well as 
Flangs immediate needs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94169

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94169: [clang][driver] Restore the original help text for `-I`

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.

Could we tweak the wording to clarify that it is Clang-specific?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94169

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94169: [clang][driver] Restore the original help text for `-I`

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm accepted this revision.
richard.barton.arm added a comment.
This revision is now accepted and ready to land.

The rationale for this revert looks good to me.

I don't see the rationale behind the original change as it was committed 
without review. If @andrei99 gets back to us then we can consider reinstating 
it, perhaps in an adjusted form.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94169

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm accepted this revision.
richard.barton.arm added a comment.

I'm happy to accept this revision based on @SouraVX 's code review and the fact 
that Andrzej has sent multiple RFCs covering the clang-side changes, including 
the Options flags (latest here 
http://lists.llvm.org/pipermail/cfe-dev/2020-October/067079.html). I think this 
code is good enough to commit.

Thanks for the code and reviews!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87989

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm accepted this revision.
richard.barton.arm added a comment.

LGTM


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

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

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm accepted this revision.
richard.barton.arm added a comment.
This revision is now accepted and ready to land.

This LGTM. I think all the previous comments from other reviewers and me have 
been dealt with so I am happy to accept this revision based on the reviews so 
far.

I have a few small inline comments to resolve in PrintHelp now we have reverted 
the functional changes there. Happy to approve on the assumption that they are 
dealt with and I don't need to see another patch, you can accept it yourself.

I think the non-flang changes to clang and llvm are in-line with what we said 
in our RFC. I think the summary of these changes are:

- Don't hard-code the name of the driver in the object constructor, pass it in 
as an argument + fix up all the clang callsites.
- Tweak the --help and --version logic to be conditional on driver mode
- Add some new FlangOption flags to Options.td
- Change the flang driver binary name to flang-new (in the already approved 
flang mods)




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
+

awarzynski wrote:
> 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.
Fair enough. Happy to cross this bridge when we come to it later on. I 
certainly think that flang-new should support -### one day.



Comment at: llvm/include/llvm/Option/OptTable.h:228
   /// \param FlagsToInclude - If non-zero, only include options with any
-  /// of these flags set.
   /// \param FlagsToExclude - Exclude options with any of these flags set.

I don't think this change is correct now that you have reverted the code change 
to the function.

If I have the same bit set in FlagsToInclude and FlagsToExclude, the 
FlagsToExclude check fires second and would mean the option is not printed. So 
FlagsToExclude takes precedence. 

Suggest to drop the edit or to correct it.



Comment at: llvm/lib/Option/OptTable.cpp:613
 unsigned Flags = getInfo(Id).Flags;
+
 if (FlagsToInclude && !(Flags & FlagsToInclude))

With the diff to this logic gone, we should also remove the new newlines so as 
not to make textual changes unrelated to this patch.



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:
> > 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 

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

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.

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


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

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

[Richard Barton via Phabricator via cfe-commits]

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 

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

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added inline comments.



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']

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.


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

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

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm requested changes to this revision.
richard.barton.arm added a comment.

A few specific comments to address here as well as the pre-commit linting ones.




Comment at: clang/lib/Driver/Driver.cpp:1584
 void Driver::PrintVersion(const Compilation , raw_ostream ) const {
+  if (IsFlangMode()) {
+OS << "Flang experimental driver (flang-new)" << '\n';

Instead of this early exit, could we instead of calling getClangFullVersion 
below call getClangToolFullVersion with a different string here?


```
if (IsFlangMode())
  OS >> getClangToolFullVersion("flang-new") << '\n';
else
  OS >> getClangFullVersion();
```

That way, we get the full clang version screen already implemented 'for free' 
and the code is nicer too (IMO)
  



Comment at: flang/CMakeLists.txt:20
 option(LINK_WITH_FIR "Link driver with FIR and LLVM" ON)
+option(BUILD_FLANG_NEW_DRIVER "Build the flang driver frontend" OFF)
 

Generally we should make sure all Flang-specific CMake build configuration 
variables start with FLANG. Suggest this is FLANG_BUILD_NEW_DRIVER or similar.



Comment at: flang/CMakeLists.txt:74
 
+
   if(LINK_WITH_FIR)

Remove



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']

I think it would be cleaner to define config.excludes unconditionally then 
append the Flang-Driver dir if our condition passes.



Comment at: flang/test/lit.cfg.py:64
 tools = [
+  ToolSubst('%flang-new', command=FindTool('flang-new'), unresolved='ignore'),
   ToolSubst('%f18', command=FindTool('f18'),

Rather than always trying to add flang-new and ignoring failure, I think it 
would be better to conditionally add this to `tools` iff we are building the 
new driver and so have `config.include_flang_new_driver_test = "ON"`. This way 
if you are building the new driver and for some reason lit fails to resolve the 
tool then you get a nice error before trying to run tests on a binary that is 
not there.



Comment at: flang/test/lit.site.cfg.py.in:13
 
+# controld the regression test for flang-new driver
+config.include_flang_new_driver_test="@BUILD_FLANG_NEW_DRIVER@"

typo "controld"



Comment at: flang/unittests/CMakeLists.txt:27
+if (BUILD_FLANG_NEW_DRIVER)
+add_subdirectory(Frontend)
+endif()

indentation?



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

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?


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

[PATCH] D73951: [Clang] [Driver]Add logic to search for flang frontend

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.

So, coming back to this after a few weeks break.

I agree with the point @awarzynski makes on this option being powerful and I 
think your alternative suggestion of using a different hard-coded name could 
work well at this stage of the project. I think the usecase that Carol and I 
were considering was for when building a clang/flang toolchain and renaming the 
binaries to something else. In that case, you would not want your driver to 
call to a hard-coded flang binary called "flang" but support calling to a flang 
binary called something else like "fooflang". In our case, we derive a 
commercial product from clang called armclang and I'm sure we are not the only 
ones doing something similar.

An additional thought I had was that we are in this situation today only 
because we need to name the binary "flang-tmp" because, as @awarzynski points 
out, "flang" is taken by the wrapper. This is because we are adding the new 
driver in the same place as the old driver, i.e. in flang/tools/f18. Firstly, 
that's a bad name for the new driver - f18 vs flang - and secondly perhaps it 
would be more clean to add the new driver as a new tool - e.g. 
flang/tools/driver and then make it a build-time configuration option which one 
to build in flang? That way we could bring the new driver up independently of 
the old driver and we would not be hitting this issue with the names. We would 
then not need this patch for the immediate issue we face today, although 
something similar may be needed to support the binary renaming case.

The other thing that has changed since the last round of review on this patch 
is that F18  is now part of the same project as 
clang, so perhaps we should be submitting both halves of this work at the same 
time.

I don't hold any of these views too strongly. I would be happy with the current 
approach or what @awarzynski proposes but have a mild preference for the 
current approach as I think it better addresses the binary renaming issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73951



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73951: [Clang] [Driver]Add logic to search for flang frontend

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added inline comments.



Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:10
+! RUN: cp %clang %t1
+! RUN: %clang --driver-mode=flang -fortran-fe %basename_t.tmp1 -### %s 2>&1 | 
FileCheck --check-prefixes=ALL %s
+

CarolineConcatto wrote:
> richard.barton.arm wrote:
> > Does %t1 not work again on this line?
> If I don't create the fake link getProgramPath will only return the name, not 
> the entire path.
> t1 here is the path for the frontend.  
> For instance:
> clang --driver-mode=flang -fortran-fe %test
> the frontend name is test, but when running it should print:
>  /test -fc1
>  without the link it will only print:
> test -fc1
> Like I said before it is more a preference that actually a requisite.
Understood - thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73951



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73951: [Clang] [Driver]Add logic to search for flang frontend

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added inline comments.



Comment at: clang/include/clang/Driver/Options.td:268
+def fortran_fe : Separate<["-"], "fortran-fe">, InternalDriverOpt,
+  HelpText<"Name for native Fortran compiler">;
 

richard.barton.arm wrote:
> This is not right. I think the option points the driver at the name of the 
> Fortran frontend to call. I don't think that has to be native or otherwise. 
> Suggest changing this string to "Name for Fortran frontend"
I prefer the new option name though - thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73951



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73951: [Clang] [Driver]Add logic to search for flang frontend

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.

Still not sure why all the tests are needed here. The first one looks fine, 
i.e. we test that --fortran-fe= 
calls to that copy. The second one appears to test the default behaviour with 
no option, but why does it do it via a different name for clang, why would that 
make a difference? The second uses a different name for both driver and 
frontend. Why is that test relevant to the change you have made? As far as I 
can see, the name of the driver binary has no bearing on its behaviour wrt. 
calling a frontend binary. What am I missing?




Comment at: clang/include/clang/Driver/Options.td:268
+def fortran_fe : Separate<["-"], "fortran-fe">, InternalDriverOpt,
+  HelpText<"Name for native Fortran compiler">;
 

This is not right. I think the option points the driver at the name of the 
Fortran frontend to call. I don't think that has to be native or otherwise. 
Suggest changing this string to "Name for Fortran frontend"



Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:10
+! RUN: cp %clang %t1
+! RUN: %clang --driver-mode=flang -fortran-fe %basename_t.tmp1 -### %s 2>&1 | 
FileCheck --check-prefixes=ALL %s
+

Does %t1 not work again on this line?



Comment at: clang/test/Driver/flang/flang-driver-2-frontend01.f90:10
+
+! The invocations should begin with .tmp1 -fc1.
+! ALL-LABEL: "{{[^"]*}}flang" "-fc1"

No they shouldn't ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73951



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73951: [Clang] [Driver]Add logic to search for flang frontend

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.

I'd still like to see the nits addressed and comments on the tests addressed 
before approving.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73951



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73951: [Clang] [Driver]Add logic to search for flang frontend

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added inline comments.



Comment at: clang/include/clang/Driver/Options.td:264
   MetaVarName<"">;
+def fcc_fortran_name : Separate<["-"], "ffc-fortran-name">, InternalDriverOpt,
+  HelpText<"Name for native Fortran compiler">;

CarolineConcatto wrote:
> richard.barton.arm wrote:
> > I'm not sure about this name. My memory is not long enough to know what ccc 
> > stands/stood for but git blame suggests it may have been a precursor name 
> > to clang. Although it might seem odd to add new options like this given the 
> > name perhaps doesn't mean anything anymore, I suggest copying this 
> > convention and make this option start with `-ccc`. I also think flang would 
> > be better than fortran here as it better describes what the option is 
> > doing, i.e. telling clang what flang is called.
> > 
> > So recommend `-ccc-flang-name` for the option and commensurate changes to 
> > the internal variables to match.
> I really don't mind changing the name. The way it is stands ffc for: flang fc1
> As I thought that ccc was for clang cc1
> As I thought that ccc was for clang cc1

Hmm - that might be right right. It is certainly as good a guess as any.
I was also thinking maybe it was "Cross CC" or something like that given the 
use of the word "native" in the help text.

Perhaps we should just not use the naming convention as we don't really 
understand it. Maybe `-flang-fc1` or `-fortran-fe` might be a better name?

Anyhow - given what you say, I'll withdraw my quibbling on the name. It is an 
internal name after all. Happy to go with whatever you chose, including the 
original.



Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:14
+! RUN: %clang --driver-mode=flang -ffc-fortran-name %basename_t.tmp1 -### %s 
2>&1 | FileCheck --check-prefixes=ALL,CHECK-EMIT-OBJ %s
+! CHECK-EMIT-OBJ-DAG: "-emit-obj"
+! CHECK-EMIT-OBJ-DAG: "-o" "{{[^"]*}}.o

CarolineConcatto wrote:
> richard.barton.arm wrote:
> > I don't understand why you are checking for this option. Surely the only 
> > relevant check is the "ALL-LABEL" check at the top which checks for the 
> > flang subprocess invocation? Can you explain why you need this check?
> Mainly for sanity check as the patch add a new flag. The logic added does not 
> mess with the previous flags, but I thought it was good to do at least a 
> minimum check.
> 
> If changing I would add more tests like the ones made in flang.f90 and 
> flang.F90 as the one in these files do not use ffc-fortran-name.
I agree with you that the check is correct, -emit-obj will indeed be emitted so 
the check is a correct one for clang's current behaviour.

Where I am coming from is that there are already tests that check that when 
clang is called without `-c` (or `-S`, etc.) that it passes `-emit-obj` to the 
frontend. This test should be checking that `-ffc-fortran-name` causes the 
frontend that clang calls to change. It's not really concerned with the 
`-emit-obj` behaviour. But, because you have this CHECK line in the code, the 
test depends on it to pass. Say we changed the spelling of that option 
-emit-obj or we changed the behaviour of clang in some way that causes 
`-emit-obj` no longer to be passed to the FE in these circumstances. This test 
would still fail if not updated, even though it is supposed to be unrelated to 
the feature being changed. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73951



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73951: [Clang] [Driver]Add logic to search for flang frontend

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm requested changes to this revision.
richard.barton.arm added a comment.

A few comments running through that need addressing.

In addition - have you checked the behaviour of this option with `-Bprefix`? 
Looking at the code for Driver.GetProgramPath, it seems like that would affect 
the behaviour of this feature so `clang --driver-mode=flang -B 
-ffc-fortran-name=foo` might be expected to make clang call `foo`. 
There also seems to be some logic with `-target` that might affect this. There 
are tests for this feature in test/Driver/B-opt.c that could be added to or 
adapted into a new test.




Comment at: clang/include/clang/Driver/Options.td:264
   MetaVarName<"">;
+def fcc_fortran_name : Separate<["-"], "ffc-fortran-name">, InternalDriverOpt,
+  HelpText<"Name for native Fortran compiler">;

I'm not sure about this name. My memory is not long enough to know what ccc 
stands/stood for but git blame suggests it may have been a precursor name to 
clang. Although it might seem odd to add new options like this given the name 
perhaps doesn't mean anything anymore, I suggest copying this convention and 
make this option start with `-ccc`. I also think flang would be better than 
fortran here as it better describes what the option is doing, i.e. telling 
clang what flang is called.

So recommend `-ccc-flang-name` for the option and commensurate changes to the 
internal variables to match.



Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:12
+! t1 is the new frontend name.
+! RUN: cp %clang %t1
+! RUN: %clang --driver-mode=flang -ffc-fortran-name %basename_t.tmp1 -### %s 
2>&1 | FileCheck --check-prefixes=ALL,CHECK-EMIT-OBJ %s

Given that the RUN line runs clang with `-###` I don't think it is necessary 
for the flang binary that `-ffc-fortran-name` points to actually be present. 
Can't we simplify the test for the basic case to run with 
`-ffc-fortran-name=foo` and check for foo?

If it is required that the `-ffc-fortran-name` argument point to an executable 
file that exists it would be better to create files in the Inputs directory and 
point there.



Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:14
+! RUN: %clang --driver-mode=flang -ffc-fortran-name %basename_t.tmp1 -### %s 
2>&1 | FileCheck --check-prefixes=ALL,CHECK-EMIT-OBJ %s
+! CHECK-EMIT-OBJ-DAG: "-emit-obj"
+! CHECK-EMIT-OBJ-DAG: "-o" "{{[^"]*}}.o

I don't understand why you are checking for this option. Surely the only 
relevant check is the "ALL-LABEL" check at the top which checks for the flang 
subprocess invocation? Can you explain why you need this check?



Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:18
+! Should end in the input file.
+! ALL: "{{.*}}clang-driver-2-frontend01.f90"{{$}}

Similarly, I don't understand the need for this check on the output, although 
it would be correct.



Comment at: clang/test/Driver/flang/driver-2-frontend01.f90:3
+
+! Driver name is a randon name. It does not contain flag, flang or clang,
+! therefore the driver invokes flang FE.

Why does the driver name matter here. Unless I have overlooked something I 
would expect the functionality to be the same whatever the driver is called.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73951



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.

I think the behaviour for missing flang is fine for now, and I think we can 
improve on it later on. 
We ought to codify (if it is not done already) where flang looks for tools to 
exec, because I think PATH is probably not the only place it could look to 
(directory of clang binary, other relative paths, other environment variables 
and commandline options, etc.) The new test will need revising once we get 
there.

I think this patch is looking good to be committed - what do you think @hfinkel 
?


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

https://reviews.llvm.org/D63607



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63607: [clang][driver] Add basic --driver-mode=flang support for fortran

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.

Thanks for the updates. I think this is now looking good and matches the RFC 
already posted.

One thought that occurs to me when reviewing this. I think we will assume that 
F18 /flang when it lands in the LLVM project will 
be an optional thing to build and will be an opt-in project at the start that 
is off by default. What happens when clang --driver-mode=flang is called and 
flang has not been built? And where would we put a test for that behaviour? If 
flang were not to be built, should --driver-mode=flang be 
unsupported/unrecognised and emit an error message?

Might not be something we need to address with this patch, but have you 
considered this?




Comment at: clang/include/clang/Driver/Driver.h:186
+  /// Other modes fall back to calling gcc which in turn calls gfortran.
+  bool IsFlangMode() const { return Mode == FlangMode; }
+

Need to update the cover letter for the patch then as it still talks about 
'fortran mode' rather than 'flang mode'.



Comment at: clang/lib/Driver/Driver.cpp:4788
+bool Driver::ShouldUseFlangCompiler(const JobAction ) const {
+  // Say "no" if there is not exactly one input of a type flang understands.
+  if (JA.size() != 1 ||

peterwaller-arm wrote:
> richard.barton.arm wrote:
> > This first clause surprised me. Is this a temporary measure or do we never 
> > intend to support compiling more than one fortran file at once?
> This function answers the question at the scope of a single JobAction. My 
> understanding is that the compiler (as with clang -cc1) will be invoked once 
> per input file.
> 
> This does not prevent multiple fortran files from being compiled with a 
> single driver invocation.
Righto - thanks for the explanation.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:44
+if (JA.getType() == types::TY_Nothing) {
+  CmdArgs.push_back("-fsyntax-only");
+} else if (JA.getType() == types::TY_LLVM_IR ||

peterwaller-arm wrote:
> richard.barton.arm wrote:
> > Looks like the F18 option spelling for this is -fparse-only? Or maybe 
> > -fdebug-semantics? I know the intention is to throw away the 'throwaway 
> > driver' in F18, so perhaps you are preparing to replace this option 
> > spelling in the throwaway driver with -fsyntax-only so this would highlight 
> > that perhaps adding the code here before the F18 driver is ready to accept 
> > it is not the right approach?
> In the RFC, the intent expressed was to mimick gfortran and clang options. I 
> am making a spelling choice here that I think it should be called 
> -fsyntax-only, which matches those. I intend to make the `flang -fc1` side 
> match in the near future.
> 
> -fdebug-* could be supported through an `-Xflang ...` option to pass debug 
> flags through.
OK - happy with this approach. So -fsyntax-only and -emit-ast will be the new 
names for f18's -fdebug-semantics and -fdebug-dump-parse-tree I guess.



Comment at: clang/lib/Driver/Types.cpp:220
+
+  case TY_Fortran: case TY_PP_Fortran:
+return true;

kiranchandramohan wrote:
> Now that there is a 2018 standard, I am assuming that f18 and F18 are also 
> valid fortran extensions. Can these be added to the File types?
> 
> Also, should the TypeInfo file be extended to handle all Fortran file types? 
> ./include/clang/Driver/Types.def
> 
> Also, should we capture some information about the standards from the 
> filename extension?
Agree with those first two, but that last one is surely a new feature that 
needs adding when such a feature is enabled in the rest of F18. 

We'd need to think that through carefully too. Classic flang never supported 
such an option and GCC's `-std` option from C/C++ does not work for Fortran. 
Also, would that go in the driver or in flang -fc1?

I suggest holding fire on this until we are ready to do it properly.



Comment at: clang/test/Driver/fortran.f95:2
+! Check that the clang driver can invoke gcc to compile Fortran when in
+! --driver-mode=clang. This is legacy behaviour - see also 
--driver-mode=fortran.
 

Need to change "see also --drver-mode=fortran" to "--driver-mode=flang" here.


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

https://reviews.llvm.org/D63607



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63607: [clang][driver] Add basic --driver-mode=fortran support for flang

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.
Herald added a subscriber: usaxena95.

Hi Peter

The overall approach seems good to me and matches how the driver is integrated 
in the original flang project so not too many surprises. I left a few comments 
mostly about the scope of the original patch. I wonder how much sense it makes 
to add support for routing options on to (f18) flang that it does not support 
yet? Would it not be better to add these options to the clang driver at the 
same time as they arrive in flang -fc1?

Ta
Rich




Comment at: clang/lib/Driver/Driver.cpp:4788
+bool Driver::ShouldUseFlangCompiler(const JobAction ) const {
+  // Say "no" if there is not exactly one input of a type flang understands.
+  if (JA.size() != 1 ||

This first clause surprised me. Is this a temporary measure or do we never 
intend to support compiling more than one fortran file at once?



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:37
+  if (isa(JA)) {
+CmdArgs.push_back("-emit-obj");
+  } else if (isa(JA)) {

F18 does not currently support these options that control the output like 
-emit-llvm and -emit-obj so this code doesn't do anything sensible at present. 
Would it not make more sense to add this later on once F18 or llvm/flang grows 
support for such options?



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:44
+if (JA.getType() == types::TY_Nothing) {
+  CmdArgs.push_back("-fsyntax-only");
+} else if (JA.getType() == types::TY_LLVM_IR ||

Looks like the F18 option spelling for this is -fparse-only? Or maybe 
-fdebug-semantics? I know the intention is to throw away the 'throwaway driver' 
in F18, so perhaps you are preparing to replace this option spelling in the 
throwaway driver with -fsyntax-only so this would highlight that perhaps adding 
the code here before the F18 driver is ready to accept it is not the right 
approach?



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:67
+  Args.AddAllArgs(CmdArgs, options::OPT_R_Group); // "Rpass-"" options 
passthrough.
+  Args.AddAllArgs(CmdArgs, options::OPT_gfortran_Group); // gfortran options 
passthrough.
+

Similarly to previous comment, do we want to be passing all gfortran options 
through to F18 in the immediate term or even the long term? I don't think there 
has been any agreement yet on what the options that F18 will support are 
(although I agree that gfortran-like options would be most sensible and in 
keeping with clang's philosophy)



Comment at: clang/test/Driver/fortran.f95:1
-// Check that the clang driver can invoke gcc to compile Fortran.
+! Check that the clang driver can invoke gcc to compile Fortran.
 

... when not in --driver-mode=fortran


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

https://reviews.llvm.org/D63607



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53210: Revert 344389 "Revert r344375 "[Driver] check for exit code from SIGPIPE""

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.

Hi @nickdesaulniers - thanks for the clarification. I was suffering from some 
PEBCAK of my own when I thought the commits were not on master. Thanks for 
these patches - a great help.


Repository:
  rC Clang

https://reviews.llvm.org/D53210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53210: Revert 344389 "Revert r344375 "[Driver] check for exit code from SIGPIPE""

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added a comment.

Hi @nickdesaulniers 
I have run into this too recently so would love to see this patch land. Did you 
get anywhere with those lld test failures?
Ta
Rich


Repository:
  rC Clang

https://reviews.llvm.org/D53210



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29773: Add support for armv7ve flag in clang (PR31358).

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm accepted this revision.
richard.barton.arm added a comment.
This revision is now accepted and ready to land.

This all LGTM. If I can assume I am allow to approve, then I approve.




Comment at: test/Preprocessor/arm-acle-6.4.c:136
+// CHECK-V7VE: __ARM_FEATURE_SIMD32 1
+// CHECK-V7VE: __ARM_FEATURE_UNALIGNED 1
+

Seems like we could check for __ARM_FEATURE_IDIV here too since we know it must 
be there.

Oh - I see it is tested in another test, in which case fine. 

I guess these test files are a bit of a mess already.


https://reviews.llvm.org/D29773



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits