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