Page MenuHomePhabricator

[flang][driver] Add -fintrinsic-modules-path option
ClosedPublic

Authored by arnamoy10 on Feb 19 2021, 12:47 PM.

Details

Summary

Add support for the following f18 options:

-fintrinsic-modules-path

Also the following changes:

  1. Create a new test case
  2. Adding dummy module files in the test directory

Depends on : D98522

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
arnamoy10 requested review of this revision.Feb 19 2021, 12:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2021, 12:47 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
arnamoy10 updated this revision to Diff 325208.Feb 20 2021, 8:06 AM

Updating the test case to include -fc1 as well

tskeith added a project: Restricted Project.Feb 20 2021, 2:55 PM
awarzynski requested changes to this revision.Feb 22 2021, 9:43 AM

Hi @arnamoy10 , thanks for the patch!

Both ieee_arithmetic.mod and iso_fortran_env.mod look like copies of similar files from <build-dir>/tools/flang/include/flang. Could you either re-use those or add something custom?

clang/include/clang/Driver/Options.td
4341

Why not copy this from gfortran?

$ gfortran --help=separate | grep intrinsic
  -fintrinsic-modules-path    Specify where to find the compiled intrinsic modules.
flang/include/flang/Frontend/PreprocessorOptions.h
31–33

As -I and -fintrinsic-modules-path are 2 different options, I would prefer to preserve that separation and model it within the driver with 2 separate vars, e.g.:

std::vector<std::string> searchDirectoriesFromDashI;
std::vector<std::string> searchDirectoriesFromIntrModPath;
48

Thank you :)

This revision now requires changes to proceed.Feb 22 2021, 9:43 AM
arnamoy10 updated this revision to Diff 326234.Feb 24 2021, 4:15 PM
arnamoy10 edited the summary of this revision. (Show Details)

Addressing comments, by separating the search directories from fintrinsic-modules-path in a separate variables. Also added dummy modules for the test case, as using %llvmshlibdir did not work out.

Also, not sure what to set as the default directory, as gfortran uses a specific installation location.

Also, not sure what to set as the default directory, as gfortran uses a specific installation location.

We probably should use a location relative to the flang-new binary, i.e. <flang-new-path>/../tools/flang/include/flang/.

Question: What are the semantics for this flag in gfortran? Is the path specified with -fintrinsics-module-path _prepended_ or _appended_ to the default search path?

Question: What are the semantics for this flag in gfortran? Is the path specified with -fintrinsics-module-path _prepended_ or _appended_ to the default search path?

I believe that it is _prepended_. Take this dummy module:

module ieee_arithmetic
type::ieee_round_type
integer(1),private::mode=0_2
end type
end

Compile it with e.g. flang-new:

flang-new -fc1 -fsyntax-only dummy-module.f90

Next, take this Fortran file:

program test
  use ieee_arithmetic
  implicit none
  real(kind=4) :: x1
  if (.not. ieee_support_nan(x1)) STOP 20
end program test

And try compiling it with gfortran:

mv ieee_arithmetic.mod tools/flang/
gfortran -fintrinsic-modules-path tools/flang/ test.f90
test.f90:2:6:

   use ieee_arithmetic
      1
Fatal Error: File ‘ieee_arithmetic.mod’ opened at (1) is not a GNU Fortran module file
compilation terminated.

So gfortran did look in tools/flang first, i.e. the path specified with -fintrinsic-modules-path.

Thank you so much for this experiment @awarzynski . I will modify the code accordingly.

arnamoy10 updated this revision to Diff 329970.Mar 11 2021, 7:59 AM
  1. Updated to set the default search directory for the intrinsic module
  2. Modified the test case to check the prepend behavior
arnamoy10 updated this revision to Diff 329976.Mar 11 2021, 8:33 AM

Clang-formatting

Thank you for updating this @arnamoy10 !

flang/lib/Frontend/CompilerInvocation.cpp
28

Not needed

293

This will not work on Windows, will it? I couldn't find any API to deal with this in a OS-agnostic way. Perhaps just remove 9 characters at the end? Or use #ifndef _WIN32?

flang/test/Driver/Inputs/ieee_arithmetic.mod
2

I think that currently the contents of this file are not relevant. So I would limit it to some helpful comment, e.g.:

Added for testing purposes. The contents of this file are currently not relevant.

Modifying the algo of default search path extraction for supporting Windows platforms.

tskeith added inline comments.Mar 11 2021, 3:09 PM
flang/lib/Frontend/CompilerInvocation.cpp
295

Does this work for an install? I think there the path would be include/flang rather than tools/flang/include/flang.

arnamoy10 added inline comments.Mar 11 2021, 3:31 PM
flang/lib/Frontend/CompilerInvocation.cpp
295

You are probably right, I am giving the path w.r.t a build. Can we make the assumption that there should be always an install? Or do we determine if we flang-new is coming from build or install (by checking if a module file is present through ls) and then set the path accordingly?

tskeith added inline comments.Fri, Mar 12, 6:25 AM
flang/lib/Frontend/CompilerInvocation.cpp
295

I think it should work in a build or an install. The flang/tools/f18/flang script checks for include/flang first and if that doesn't exist it uses tools/flang/include/flang so you could do the same. It would be good if we could fix the build or install so that the location is consistent.

tskeith added inline comments.Fri, Mar 12, 9:36 AM
flang/lib/Frontend/CompilerInvocation.cpp
295

I've created D98522 to make the relative path be include/flang in both build and install. If no one objects, that should make it so you don't need conditional code here, just use "/../include/flang".

arnamoy10 added inline comments.Fri, Mar 12, 9:42 AM
flang/lib/Frontend/CompilerInvocation.cpp
295

Thanks for doing this

arnamoy10 edited the summary of this revision. (Show Details)

Update the path based on the patch D98522

bryanpkc added inline comments.
flang/lib/Frontend/CompilerInvocation.cpp
294

Can you use llvm::sys::path::remove_filename here?

I tried reproducing the CI failures reported at the top (in particular intrinsic_module_path.f90), but haven't had much luck. @arnamoy10 - how about you?

Could you clang-format this patch when uploading the next diff? Thank you!

flang/lib/Frontend/CompilerInvocation.cpp
323

This TODO has been addressed, right?

327

This method is meant for parsing input parameters, but the default directory for intrinsic modules is not a parameter. I think that setFortranOpts would be a better place for adding the default directory for fortranOptions.searchDirectories.

Clang-formatting and addressing reviewers' comments:

Moved the default directory append to setFortranOpts()

arnamoy10 updated this revision to Diff 330785.Mon, Mar 15, 1:24 PM

Adding the dummy modules

arnamoy10 updated this revision to Diff 330968.Tue, Mar 16, 6:42 AM

Adding the test file

arnamoy10 updated this revision to Diff 331528.Thu, Mar 18, 5:31 AM

Fixing bug in test case

arnamoy10 added inline comments.Thu, Mar 18, 6:27 AM
flang/lib/Frontend/CompilerInvocation.cpp
294

Thank you. This seems like a better option, but I could not make it work. For the sake of time I am keeping this hard coded manipulation. It can be improved in a later revision.

awarzynski added inline comments.Fri, Mar 19, 7:17 AM
flang/lib/Frontend/CompilerInvocation.cpp
294

It would be really nice to use llvm::sys::path::remove_filename instead. Perhaps something like this:

llvm::SmallString<128> driverPath;
driverPath.assign(llvm::sys::fs::getMainExecutable(nullptr, nullptr));
llvm::sys::path::remove_filename(driverPath);

?

arnamoy10 updated this revision to Diff 331888.Fri, Mar 19, 8:29 AM

Using llvm::sys::path::remove_filename as per suggestion.

flang/lib/Frontend/CompilerInvocation.cpp
294

This works, thanks for the code!

awarzynski added inline comments.Fri, Mar 19, 10:11 AM
flang/lib/Frontend/CompilerInvocation.cpp
297

Given this conversion operator, wouldn't return std::string(driverPath); be more efficient?

Addressing reviewers' comments

flang/lib/Frontend/CompilerInvocation.cpp
297

Done, thanks

awarzynski accepted this revision.Sat, Mar 20, 7:38 AM

LGTM, thank you for working on this!

In the summary you refer to -fdebug-module-writer. Could you please update before merging this?

flang/lib/Frontend/CompilerInvocation.cpp
28

Please revert before merging.

322

[nit] To me, currently hardcoded suggests that it's a temporary solution. But that's not the case, is it? Also, the default location is relative to the location of the driver, so it's not hardcoded.

This revision is now accepted and ready to land.Sat, Mar 20, 7:38 AM
arnamoy10 edited the summary of this revision. (Show Details)Tue, Mar 23, 5:51 AM
arnamoy10 updated this revision to Diff 332649.Tue, Mar 23, 6:15 AM

Addressing minor issues

This revision was landed with ongoing or failed builds.Tue, Mar 23, 9:30 AM
This revision was automatically updated to reflect the committed changes.