This is an archive of the discontinued LLVM Phabricator instance.

[flang][driver]Add experimental flang driver and frontend with help screen
ClosedPublic

Authored by awarzynski on Aug 17 2020, 9:20 AM.

Details

Summary

This is the first patch implementing the new Flang driver as outlined in [1],
[2] & [3]. It created Flang driver (flang-new and Flang frontend driver
(flang-new -fc1) with help screen(-help) and version(--version) options.
These will be renamed as flang and flang -fc1 once the current Flang
throwaway driver, flang, can be replaced

flang-new is implemented in terms of libclangDriver, defaulting the driver
mode to flang (added to libclangDriver in [2]). This ensures that the driver
runs in flang mode regardless of the name of the binary inferred from argv[0].

The design of the new flang compiler and frontend drivers is inspired by it
counterparts in Clang [3]. Currently, the new flang compiler and frontend
drivers re-use Clang libraries: clangBasic, clangDriver and clangFrontend.
To identify Flang options, this patch adds FlangOption enum. printHelp is
updated so that flang-new prints only Flang options.

The new Flang driver is disabled by default. To build it, set
-DBUILD_FLANG_NEW_DRIVER =ON when configuring Flang and LLVM_ENABLE_PROJECTS
with clang.

[1] RFC: new Flang driver - next steps
http://lists.llvm.org/pipermail/flang-dev/2020-July/000470.html
[2] "RFC: Adding a fortran mode to the clang driver for flang"
http://lists.llvm.org/pipermail/cfe-dev/2019-June/062669.html
[3]RFC: refactoring libclangDriver/libclangFrontend to share with Flan
http://lists.llvm.org/pipermail/cfe-dev/2020-July/066393.html

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
CarolineConcatto requested review of this revision.Aug 17 2020, 9:20 AM
CarolineConcatto edited the summary of this revision. (Show Details)
CarolineConcatto retitled this revision from Add experimental flang driver and frontend with help screen to [flang][driver]Add experimental flang driver and frontend with help screen.Aug 17 2020, 9:24 AM
CarolineConcatto added a project: Restricted Project.
tskeith requested changes to this revision.Aug 17 2020, 10:35 AM
tskeith added a subscriber: tskeith.
tskeith added inline comments.
flang/include/flang/Frontend/CompilerInstance.h
12

Why is this called "Frontend" rather than "Driver"?

17

Nothing else is in namespace flang. Fortran::frontend would be more consistent.

22

Data members, local variables, etc. should start with lower case.
Non-public data members should end with underscore.

Please follow the style in flang/documentation/C++style.md.

flang/include/flang/Frontend/FrontendOptions.h
32

Why isn't the type Format?

33

Why isn't the type bool?

66

Why aren't the types bool?

flang/include/flang/FrontendTool/Utils.h
19

Is this needed?

flang/lib/Frontend/CompilerInstance.cpp
40

The "then" and "else" statements should have braces around them.

flang/lib/Frontend/CompilerInvocation.cpp
47

Why not return InputKind{Language::Unknown};?

92

What is the purpose of DashX here?

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
21

What is this for?

flang/tools/flang-driver/driver.cpp
31

Why isn't this declared in a header?

flang/tools/flang-driver/fc1_main.cpp
33

MainAddr isn't used.

55

Why is this statement in a block? Is the result of CreateFromArgs supposed to affect the return value of this function?

This revision now requires changes to proceed.Aug 17 2020, 10:35 AM
richard.barton.arm requested changes to this revision.Aug 20 2020, 7:14 AM

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

clang/lib/Driver/Driver.cpp
1584

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)

flang/CMakeLists.txt
20

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

86

Remove

flang/test/lit.cfg.py
40

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

68

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.

flang/test/lit.site.cfg.py.in
14

typo "controld"

flang/unittests/CMakeLists.txt
27

indentation?

llvm/lib/Option/OptTable.cpp
621 ↗(On Diff #286047)

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?

Thanks for the work.

A couple of comments on clang/ related changes:
An out-of-tree build with this patch fails for me:
Here's what I did:
I initially used ENABLE_PROJECTS="clang;mlir" to build llvm-project, I didn't build flang during this run.

Then I passed following to the cmake when building flang as out of tree.

cmake -G Ninja \
  -DCMAKE_BUILD_TYPE=Release \
  -DFLANG_ENABLE_WERROR=On \
  -DCMAKE_CXX_STANDARD=17 \
  -DLLVM_TARGETS_TO_BUILD=host \
  -DLLVM_LIT_ARGS=-v \
  -D LLVM_DIR=${LLVM_MLIR_CLANG_BUILD}/lib/cmake/llvm \
  -D MLIR_DIR=${LLVM_MLIR_CLANG_BUILD}/lib/cmake/mlir \
  -DBUILD_FLANG_NEW_DRIVER=ON \
  ../
ninja -j16

where LLVM_MLIR_CLANG_BUILD points to initially built llvm-project .
I see below error message:

../lib/Frontend/CompilerInvocation.cpp: In static member function ‘static bool flang::CompilerInvocation::CreateFromArgs(flang::CompilerInvocation&, llvm::ArrayRef<const char*>, clang::DiagnosticsEngine&)’:
../lib/Frontend/CompilerInvocation.cpp:85:67: error: ‘FlangOption’ is not a member of ‘clang::driver::options’
       clang::driver::options::CC1Option | clang::driver::options::FlangOption;
                                                                   ^~~~~~~~~~~
../lib/Frontend/CompilerInvocation.cpp:85:67: note: suggested alternative: ‘LastOption’
       clang::driver::options::CC1Option | clang::driver::options::FlangOption;
                                                                   ^~~~~~~~~~~
                                                                   LastOption

Do you need to add -DCLANG_DIR flag, as there seem to be a dependency for this patch on clang as libraries?

clang/include/clang/Driver/Options.td
60

nit: a missing period.
flang mode -> flang mode.

2110

nit: - A space before FlangOption
CC1AsOption,FlangOption -> CC1AsOption, FlangOption

3059–3060

Same as above.
nit: - A space before FlangOption

clang/lib/Driver/Driver.cpp
1574

Can IsFlangMode() be used instead?

1578

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?

kadircet removed a subscriber: kadircet.Aug 20 2020, 1:53 PM
flang/test/lit.cfg.py
40

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.

sameeranjoshi requested changes to this revision.Aug 21 2020, 11:24 AM

Thanks for working on it.
A few review comments/questions on changes in flang part from the patch.

flang/include/flang/Frontend/CompilerInstance.h
94

The block of comments above make sense for this function and not the currently mentioned one.
Please interchange/replace the comments to later declared function.
Wrong comments above could reflect in doxygen APIs, misleading the reader of code.

flang/include/flang/FrontendTool/Utils.h
2

nit:: blank line.

sameeranjoshi added inline comments.Aug 21 2020, 11:24 AM
flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
36

With

clang --version
clang version 11.0.0 (https://github.com/llvm/llvm-project.git 1acf129bcf9a1b51e301a9fef151254ec4c7ec43)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/local/bin

whereas with

./bin/flang-new --vesion
Flang experimental driver (flang-new)

I see both clang & flang call
llvm::cl::PrintVersionMessage() internally.

Is more information need to be registered in llvm(llvm::cl) for flang to give more detailed output or will that come later once we start adding more patch for driver?

40

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.

Address review comments

Replace namespace flang for Fortran::frontend
and fix style

CarolineConcatto marked 17 inline comments as done.Aug 23 2020, 1:55 PM

Hey Folks,
Fist of all, thank you very much for your review.
I have answered some questions and update some of the requests.
There are some that I did not had time to go inside yet.
I have the week of 24th off. IN my return I will tackle all the reviews as:
out of build tree
and some question about the driver

flang/include/flang/Frontend/CompilerInstance.h
12

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.

22

I believe I have changed the style of the patch to be flang style.

flang/include/flang/Frontend/FrontendOptions.h
32

This is to not mistake with: enum Format { Source, ModuleMap, Precompiled };

33

Most of these things are going to be set by option::driver. That is the reason it is as it is.

66

Most of these things are going to be set by option::driver. That is the reason it is as it is.

flang/lib/Frontend/CompilerInvocation.cpp
47

Just because the next lines also return DashX.

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
21

Because of CompilerInstance *Flang.

flang/test/lit.cfg.py
40

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.

SouraVX added inline comments.Aug 23 2020, 10:53 PM
flang/tools/flang-driver/fc1_main.cpp
2

This file-name seems odd considering LLVM style. How about just Flang.cpp or FlangMain.cpp ? or other

@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.

clang/lib/Driver/Driver.cpp
1578

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 ;-)

flang/include/flang/Frontend/CompilerInstance.h
12

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)

flang/include/flang/Frontend/FrontendOptions.h
32–33

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.

66

Why aren't the types bool?

@tskeith See above.

flang/lib/Frontend/CompilerInvocation.cpp
47

I agree with @tskeith , return InputKind{Language::Unknown}; would be cleaner. I will update this.

92

Not needed for now, I will delete it. Thanks!

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
21

It's not needed, good catch, cheers!

40

@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.

flang/test/lit.cfg.py
40

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.

flang/test/lit.site.cfg.py.in
15
flang/tools/flang-driver/driver.cpp
31

We'd be creating a dedicated header file for no real benefit (this method should not be re-used anywhere else).

flang/tools/flang-driver/fc1_main.cpp
2

This is for consistency with clang: https://github.com/llvm/llvm-project/tree/master/clang/tools/driver. Perhaps Fc1Main.cpp instead?

55

Copy and paste error - cheers for pointing out, fixed in the next revision!

llvm/lib/Option/OptTable.cpp
621 ↗(On Diff #286047)

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).

andwar updated this revision to Diff 288018.Aug 26 2020, 9:55 AM
andwar added a subscriber: andwar.
  • Canonicalised FLANG_BUILD_NEW_DRIVER in flang/test/CMakeLists.txt and updated the LIT config scripts accordingly
  • Implemented proper handling of -emit-obj (via diagnostics) in CompilerInvocation.cpp
  • Removed members from InputKind which are not needed (and the related methods)
  • Removed/updated comments, added new comments to clarify the design and to highlight the future steps
  • Simplified the original change in OptTable::PrintHelp (made it more generic)
  • Removed unused #includes
  • Fine-tuned the tests (I wanted to make sure that they clearly communicate what is currently supported and what is not)
  • Addressed all the outstanding comments

@andwar is my Phabricator alter-ego. Apologies for the confusion - I had my Arcanist misconfigured.

Added support for out-of-tree builds. It requires -DCLANG_DIR=<llvm-buuld-dir>/lib/cmake/clang when configuring CMake. That's on top of LLVM_DIR and MLIR_DIR.

awarzynski commandeered this revision.Aug 27 2020, 12:20 PM
awarzynski added a reviewer: CarolineConcatto.

@sameeranjoshi Thanks for testing out-of-tree builds. We were missing some minor changes in CMake. I have now uploaded these. Out-of-tree will require -DCLANG_DIR when configuring build while the new driver depends on Clang.

As for your original issue - I suspect that you forgot to update your in-tree directory. This patch needs to be applied both in-tree (in llvm-project) and out-of-tree (flang).

sameeranjoshi accepted this revision.Aug 31 2020, 4:14 AM

Thank you for changes.
I was able to build successfully out-of-tree.
Please update the README.md with the necessary changes.

LGTM!

awarzynski updated this revision to Diff 289111.Sep 1 2020, 2:50 AM
awarzynski marked an inline comment as done.

Update README.md with instructions for building flang-new

richard.barton.arm requested changes to this revision.Sep 2 2020, 11:01 AM

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?
flang/test/CMakeLists.txt
12

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).

flang/test/Flang-Driver/driver-error-cc1.c
8

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.

flang/test/Flang-Driver/driver-error-cc1.cpp
2

Same comment as above on exit code.

flang/test/Flang-Driver/emit-obj.f90
3

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.

flang/test/lit.cfg.py
39

This comment should be on line 41

40

Happy with this approach :-)

flang/test/lit.site.cfg.py.in
15

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.

llvm/lib/Option/OptTable.cpp
621 ↗(On Diff #286047)

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.

This revision now requires changes to proceed.Sep 2 2020, 11:01 AM

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

awarzynski marked 2 inline comments as done.
  • Adddressed comments from @richard.barton.arm
  • Added FC1Option to ClangFlags (and made other related changes)
  • Added missing code to check return codes from subcommands (flang-new)

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

flang/test/CMakeLists.txt
12

AFAIK, LINK_WITH_FIR is not used in LIT tests.

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

flang/test/Flang-Driver/driver-error-cc1.c
8

Good catch, thanks! Fixed in the latest revision.

flang/test/Flang-Driver/driver-error-cc1.cpp
2

Good catch, thanks! Fixed in the latest revision.

flang/test/Flang-Driver/emit-obj.f90
3

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.

flang/test/lit.cfg.py
39

Updated, ta!

flang/test/lit.site.cfg.py.in
15

I did not know that one - thanks! Updated.

llvm/lib/Option/OptTable.cpp
621 ↗(On Diff #286047)

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!

awarzynski updated this revision to Diff 290435.Sep 8 2020, 2:43 AM

Add missing update in lit.cfg.py

richard.barton.arm accepted this revision.EditedSep 10 2020, 4:14 AM

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)

I am happy to approve these in good faith.

flang/test/Flang-Driver/emit-obj.f90
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.

llvm/include/llvm/Option/OptTable.h
246

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.

llvm/lib/Option/OptTable.cpp
613 ↗(On Diff #290435)

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.

621 ↗(On Diff #286047)

Thanks for the explanation. I agree that it is best to wait for us to have a concrete motivating example before making this tweak to the PrintHelp logic. It will make it easier to test for one thing and will also make the patches more logically consistent.

This revision is now accepted and ready to land.Sep 10 2020, 4:14 AM
awarzynski updated this revision to Diff 291068.EditedSep 10 2020, 12:48 PM
awarzynski marked 2 inline comments as done.

Address final comments + rebase

I also took the liberty and made the following changes in CMake scripts:

  • replaced add_library wiht add_flang_library
  • replaced add_executable with add_flang_tool

These are all NFCs.

This revision was landed with ongoing or failed builds.Sep 11 2020, 3:00 AM
This revision was automatically updated to reflect the committed changes.

We forgot to update the signature and definition of OptTable::findByPrefix: https://github.com/llvm/llvm-project/blob/257b29715bb27b7d9f6c3c40c481b6a4af0b37e5/llvm/include/llvm/Option/OptTable.h#L154-L155. That's required after updating the definition of OptTable::Flags. Not sure none of the compilers complained.

Fixed here: https://github.com/llvm/llvm-project/commit/4eed800b18abaeba3082bf950fbe5c3020c4b592