Page MenuHomePhabricator

[flang][driver] Add support for `-J/-module-dir`
ClosedPublic

Authored by arnamoy10 on Jan 26 2021, 8:02 AM.

Details

Summary

Add support for option -J/-module-dir in the new Flang driver.
This will allow for including module files in other directories, as the default search path is currently the working folder. This also provides an option of storing the output module in the specified folder.

Diff Detail

Event Timeline

arnamoy10 created this revision.Jan 26 2021, 8:02 AM
arnamoy10 requested review of this revision.Jan 26 2021, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2021, 8:02 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
awarzynski added a project: Restricted Project.Jan 26 2021, 11:29 AM

@arnamoy10, thank you for this patch and for working on this! I have a few high-level suggestions (+ some inline comments):

Q1
-module-dir and -J in Options.td should be aliases - otherwise we need to duplicate some code. I've not used Aliases myself, but perhaps this example could be helpful. You might discover that supporting 2 different spellings for one options is currently not possible. In such case we should start with one spelling.

Q2
Could you try moving your changes from FrontendActions.cpp to CompilerInvocation.cpp/CompilerInstance.cpp? Ideally, CompilerInvocation should encapsulate all compiler and langauge options relevant to the current invocation. Once we enter FrontendActions::ExecuteAction, all of them should be set and ready. This way ExecuteAction focuses on the action itself rather than setting it up. Also, adding Fortran::semantics::SemanticsContext to CompilerInstance could help with encapsulation.

Q3
What about help text for -J?

Thank you, this is looking really good and it's great to see more people working on the new driver!

clang/include/clang/Driver/Options.td
952–953

As we are trying to follow gfortran, I suggest that we copy the help message from there:

$ gfortran --help=separate | grep '\-J'
  -J<directory>               Put MODULE files in 'directory'

Also, we can add the long version (via DocBrief field) from here https://gcc.gnu.org/onlinedocs/gfortran/Directory-Options.html:

This option specifies where to put .mod files for compiled modules. It is also added to the list of directories to searched by an USE statement.

The default is the current directory.

I appreciate that this patch only implements the 2nd part of what the option is intended to offer (i.e. updates the search patch for module files). But I think that it's worthwhile to make the intent behind this option clear from the very beginning. We can use the commit message to document the current limitations.

Also, please keep in mind that this help message is going to be re-used by -J, which belongs to gfortran_Group. So the description needs to be valid for both.

clang/lib/Driver/ToolChains/Flang.cpp
31

Are -J and -module-dir really preprocessor options? Wouldn't OPT_J and OPT_module_dir be better suited in ConstructJob (or some other method for other options)?

107

This is an unrelated change. I'm fine with this, but ideally as a separate NFC patch (otherwise the history is distorted).

flang/include/flang/Frontend/PreprocessorOptions.h
34 ↗(On Diff #319298)

IMHO searchDirectoryFromDashJ would be::

  • more consistent (matches searchDirectoryFromDashI)
  • more accurate (internally this is only used for defining search directories)
flang/include/flang/Parser/parsing.h
35 ↗(On Diff #319298)

This is merely module _search_ directory, right? Perhaps worth renaming as moduleSearchDirectory?

Also, is it required by the parser? IIUC, it's only used when calling set_moduleDirectory, which is part of Fortran::semantics::SemanticsContext.

flang/lib/Frontend/CompilerInvocation.cpp
224

searchDirectoriesFromDashI contains search directories specified with -I. If we add things from -J then the name is no longer valid :) One option is to rename the variable. Personally I think that we can skip this line.

251

Unrelated change

flang/lib/Frontend/FrontendActions.cpp
133

I think that we can safely set a default value for moduleDirectory and skip this check. From https://gcc.gnu.org/onlinedocs/gfortran/Directory-Options.html:

The default is the current directory.
flang/test/Flang-Driver/include-module.f90
10

What about:

  • -J %S/Inputs -J %S/Inputs/module-dir (i.e. -J + -J)
  • -J %S/Inputs -module-dir %S/Inputs/module-dir (i.e. -J + -module-dir)
  • -module-dir %S/Inputs -J%S/Inputs/module-dir (i.e. -module-dir + -J)
  • -module-dir %S/Inputs -I%S/Inputs/module-dir (.e. -module-dir + -I)

I appreciate that this is a bit tedious, but IMO it is worthwhile to add a few more cases here to avoid any unpleasant breakage in the future. Also - what should the relation between -I and -J be here? As in, which ought to have higher priority?

@arnamoy10 , could you also add tests that show that -J/-module-dir is taken into account when deciding where to put module files? Thank you!

Addressing reviewers' comments with the following changes:

  1. Aliasing of -module-dir and -J to avoid code duplication
  2. Moving the code to set the module and search directories from FrontendActions.cpp to CompilerInvocation.cpp
  3. Data structures updated/ variables added to separate Preprocessor options from Compiler Options
  4. Added more test cases.

Added test for the driver-help, also contains all the changes that was done in the previous diff (Aliasing -J and -module-dir, changing data structures etc).

tskeith added inline comments.Jan 29 2021, 2:06 PM
clang/include/clang/Driver/Options.td
956

It would be better to make -module-dir the main option and -J the alias. -J is only there for gfortran compatibility, not because it is a good name for the option.

flang/include/flang/Frontend/CompilerInstance.h
105

semanticsContext would be a better name for this function.

flang/include/flang/Frontend/CompilerInvocation.h
67

moduleDir_ would be a better name.

flang/lib/Frontend/CompilerInstance.cpp
28

Why is semantics_ a shared_ptr rather than a simple data member of type SemanticsContext? It's owned by only CompilerInstance, not shared.
The same question probably applies to the other fields too.

arnamoy10 updated this revision to Diff 320306.Jan 30 2021, 9:00 AM

Addressing reviewers' comments, adding -module-dir as the default option instead of -J

@arnamoy10 Thank you for addressing my comments!

As for testing that -J/-module-dir are taken into account when specifying the output directory for modules, could try adding the following:

! RUN: mkdir -p %t/dir-f18 && %f18 -fparse-only -I tools/flang/include/flang -module %t/dir-f18 %s  2>&1
! RUN: ls %t/dir/testmodule.mod && not ls %t/testmodule.mod

! RUN: mkdir -p %t/dir-flang-new && %flang-new -fsyntax-only -module-dir %t/dir-flang-new %s  2>&1
! RUN: ls %t/dir/testmodule.mod && not ls %t/testmodule.mod

module testmodule
  type::t2
  end type
end

It's possible that flang-new doesn't support writing modules yet, but IMHO this is the right moment to try and understand what might be missing. Thank you!

clang/include/clang/Driver/Options.td
952–953

No indentation in DocBrief, see this example

Also, Put MODULE files in 'directory' -> Put MODULE files in <dir> (the option is displayed as -module-dir <dir>).

4124

There's no need to move this, is there? I feel that it's better to keep all gfortran options together.

flang/include/flang/Frontend/CompilerInstance.h
105

We should follow Flang's coding style here:

Accessor member functions are named with the non-public data member's name, less the trailing underscore.

i.e. semantics() rather than semanticsContext(). If we were to diverge from that, then I suggest that we follow the style prevalent in LLVM/Clang, see e.g. getSema.

@tskeith, I'm guessing that you wanted the member variable to be updated too:

  • semantics_ -> semanticsContext_

Makes sense to me.

flang/lib/Frontend/CompilerInstance.cpp
28

@tskeith You raise two important points here:

Why shared_ptr if the resource is not shared?
From what I can see, at this point the SemanticsContext is not shared and we can safely use unique_ptr instead.

Why are semantics_ and other members of CompilerInstance pointers?
CompilerInstance doesn't really own much - it just encapsulates all classes/structs that are required for creating a _compiler instance_. It's kept lightweight and written in a way that makes it easy to _inject_ custom instances of these classes. This approach is expected to be helpful when creating new frontend actions (I expect that there will be a lot) and when compiling projects with many source files.

@tskeith, I intend to document the design of the new driver soon and suggest that that's when we re-open the discussion on the design of CompilerInstance.

IMO this change is consistent with the current design and I think that we should accept it as is.

Small suggestion
@arnamoy10 , I think that you can safely add IntrinsicTypeDefaultKinds and LanguageFeatureControl members too. We will need them shortly and this way this constructor becomes much cleaner. I'm fine either way!

flang/lib/Frontend/CompilerInvocation.cpp
357

The argument is not needed here, is it? You could just write:

auto &semaCtx = semanticsContext()

Or am I missing something?

flang/lib/Frontend/FrontendActions.cpp
130–131

[nit] Unrelated change

flang/test/Flang-Driver/include-module.f90
14

Why --check-prefix=SINGLEINCLUDE here and below? Both directories are included, so there should be no errors.

awarzynski added inline comments.Feb 1 2021, 6:02 AM
flang/test/Flang-Driver/include-module.f90
14

Apologies, I was wrong with regard to how -J/-module-dir should work:

$ gfortran -J test-dir/ -J test-dir/ test.f
f951: Fatal Error: gfortran: Only one ‘-J’ option allowed
compilation terminated.

gfortran behavior makes a lot of sense to me and I suggest that we replicate that. This means that we should issue a diagnostic when -J/-module-dir is used twice.

tskeith added inline comments.Feb 1 2021, 8:12 AM
flang/include/flang/Frontend/CompilerInstance.h
105

@tskeith, I'm guessing that you wanted the member variable to be updated too:

  • semantics_ -> semanticsContext_

Right.

flang/lib/Frontend/CompilerInstance.cpp
28

Why are semantics_ and other members of CompilerInstance pointers?
CompilerInstance doesn't really own much - it just encapsulates all classes/structs that are required for creating a _compiler instance_.

As it stands it does own the instance of SemanticsContext etc. No one else does.

@tskeith, I intend to document the design of the new driver soon and suggest that that's when we re-open the discussion on the design of CompilerInstance.

OK

arnamoy10 updated this revision to Diff 320617.Feb 1 2021, 3:14 PM

A few more comments addressed and a new test case added for write-module check.

LGTM!

Thank you for addressing my comments, (DELETEME can be fixed when pushing upstream)! From what I can see you've also addressed all of Tim's comments, but could you wait a day or two before merging this? Just in case I missed something, or Tim or somebody else has further comments.

flang/lib/Frontend/CompilerInvocation.cpp
229–230

DELETEME

flang/lib/Frontend/FrontendActions.cpp
111–112

DELETEME

arnamoy10 added a comment.EditedFeb 2 2021, 7:01 AM

Sounds good to me, thanks for the help. In the meantime I will work on the fdefault-* family of flags, which will be dependent on this patch I think.

PS: Please disregard the below comments, those were drafts, some of them are invalid, sorry about that.

clang/include/clang/Driver/Options.td
956

Sure, we can do that. I just chose -J to be default as it is easier to type for the user :)

4124

This needs to be moved, as we are using Aliases. The way Aliases work is (in this order) (1) you create the base option (that

flang/lib/Frontend/CompilerInstance.cpp
28

No idea about this design decision. I have not found any other DS that is "sharing" these pointers. I can take them out.

flang/lib/Frontend/CompilerInvocation.cpp
357

semanticsContext_ belongs to CompilerInstance, not CompilerInvocation, so unfortunately that is not possible.

flang/test/Flang-Driver/include-module.f90
10

Done

awarzynski accepted this revision.Feb 4 2021, 8:13 AM
This revision is now accepted and ready to land.Feb 4 2021, 8:13 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.
clang/include/clang/Driver/Options.td
952–953

Is the clang file change needed?

clang -help now has -module-dir <dir> Put MODULE files in <dir> while I think the option only makes sense for flang.

walli99 added a subscriber: walli99.Thu, Jun 3, 3:56 PM