Page MenuHomePhabricator

[flang][driver] Add options for -fdefault* and -flarge-sizes
ClosedPublic

Authored by arnamoy10 on Feb 9 2021, 7:39 AM.

Details

Summary

Add support for the following Fortran dialect options:

  • -default*
  • -flarge-sizes

It also adds two test cases:

  1. For checking whether flang-new is passing options correctly to flang-new -fc1.
  2. For checking if fdefault- arguments are processed properly.

Also moves the Dialect related option parsing to a dedicated function and adds a member defaultKinds() to CompilerInvocation

Depends on: D96032

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 9 2021, 7:39 AM
Herald added a project: Restricted Project. · View Herald Transcript

Hi @arnamoy10, thank you for working on this! Overall this looks good to me, but I'm wondering whether we could simplify it a bit? More comments inline.

clang/include/clang/Driver/Options.td
4240–4241

Could you add a help text? Note that once you add a help text, you will have to add FlangOnlyOption flag to make sure this option doesn't show up in:

clang -help

Some for other options here. Also, at that point, I suggest moving these options near other Flang options: https://github.com/llvm/llvm-project/blob/ec4fb5bcd3b92867156a5bd75fa0be4c74084f3c/clang/include/clang/Driver/Options.td#L4224-L4238.

AFAIK, these options are no longer forwarded to gfortran anyway (since this patch).

4243

Is this option available in gfortran? If not we shouldn't be adding Group<gfortran_Group> here.

flang/include/flang/Frontend/CompilerInvocation.h
22–28

Do we really need this? if yes, FrontendOptions.h would probably be a better place for this.

73–74

Wouldn't it be possible to just use defaultKinds_ here rather than defaultKinds_ _and_ dialectOpts_? Do you think that we will need both?

flang/lib/Frontend/CompilerInvocation.cpp
325

Is this requirement document anywhere?

flang/test/Flang-Driver/pipeline.f90
1–2 ↗(On Diff #322390)

I suspect that this is C&P error from here :)

IIUC, this file tests that the compiler driver (flang-new) forwards all Flang frontend options to the compiler frontend driver (flang-new -fc1) as expected. Could you update the description?

arnamoy10 added inline comments.Feb 11 2021, 2:29 PM
flang/lib/Frontend/CompilerInvocation.cpp
325

Just found out through running gfortran

arnamoy10 added inline comments.Feb 12 2021, 4:25 AM
clang/include/clang/Driver/Options.td
4240–4241

What should be the help text for -flarge-sizes, given we cannot take reference from gfortran

Btw, could you clang-format your patch? I normally use git -lang-format HEAD~. Thank you!

flang/lib/Frontend/CompilerInvocation.cpp
325

Oh, indeed:

gfortran -fdefault-double-8 file.f
f951: Fatal Error: Use of ‘-fdefault-double-8’ requires ‘-fdefault-real-8’
compilation terminated.

Thanks you for being so thorough and checking this! :)

I think that it would help to:

  • add a comment explaining _why_ this diagnostic is added (compatibility with gfortran is a good reason IMO)
  • add a test to verify that it's indeed issued.
arnamoy10 updated this revision to Diff 323480.Feb 12 2021, 2:26 PM
arnamoy10 edited the summary of this revision. (Show Details)

Addressing reviewers' comments.

LGTM, thank you for working on this @arnamoy10 !

I left a few _minor_ comments. These can be addressed in the actual commit.

clang/include/clang/Driver/Options.td
4354–4355

As these options are defined within this let statement:

let Flags = [FC1Option, FlangOption, FlangOnlyOption] in {

}

there is no need to specify Flags independently for each.

flang/include/flang/Frontend/CompilerInvocation.h
73–74

Thank you for updating this.

One additional point - I would be tempted to use a regular member variable here rather than a pointer. I suspect that you were inspired by semanticContext_? Currently we have to use a pointer for semanticsContext_ as we can't use the default constructor for SemanticsContext (it requires arguments that we don't have access to at the point of constructing CompilerInvocation). That's not the case for IntrinsicTypeDefaultKinds.

But I might missing something here!

flang/test/Flang-Driver/driver-help.f90
34

[nit] Unrelated change

awarzynski accepted this revision.Feb 15 2021, 4:28 AM
This revision is now accepted and ready to land.Feb 15 2021, 4:28 AM
arnamoy10 updated this revision to Diff 323746.Feb 15 2021, 7:28 AM

Addressed latest reviewer's comments, no longer using a pointer for defaultKinds

tskeith added inline comments.Feb 15 2021, 8:23 AM
clang/include/clang/Driver/Options.td
4361

That's not what -flarge-sizes does. Here is the description from flang/docs/Extensions.md:

The default INTEGER type is required by the standard to occupy
the same amount of storage as the default REAL type. Default
REAL is of course 32-bit IEEE-754 floating-point today. This legacy
rule imposes an artificially small constraint in some cases
where Fortran mandates that something have the default INTEGER
type: specifically, the results of references to the intrinsic functions
SIZE, STORAGE_SIZE,LBOUND, UBOUND, SHAPE, and the location reductions
FINDLOC, MAXLOC, and MINLOC in the absence of an explicit
KIND= actual argument. We return INTEGER(KIND=8) by default in
these cases when the -flarge-sizes option is enabled.
SIZEOF and C_SIZEOF always return INTEGER(KIND=8).

flang/lib/Frontend/CompilerInvocation.cpp
333

If -fdefault-double-8 requires -fdefault-real-8 then why is the default real kind set to 4?

I don't understand the purpose of this option. The kind of DOUBLE PRECISION is already 8 so there is no need to change it.

awarzynski added inline comments.Feb 16 2021, 1:04 AM
clang/include/clang/Driver/Options.td
4361

It will be tricky to capture all that in a short help text. Would this work:

Use INTEGER(KIND=8) for the result type in size-related intrinsics.

?

arnamoy10 updated this revision to Diff 324296.Feb 17 2021, 6:54 AM

Addresses proper handling of sizes

awarzynski requested changes to this revision.Feb 17 2021, 8:06 AM

Thank you for updating this!

I've left a couple of small suggestions inline. Once those are addressed I believe that this is ready to land. @tskeith, any thoughts ?

flang/lib/Frontend/CompilerInvocation.cpp
318

From gfortran documentation:

This option promotes the default width of DOUBLE PRECISION and double real constants like 1.d0 to 16 bytes if possible.

So I believe that you are missing:

res.defaultKinds().set_doublePrecisionKind(16);
333–335

I think that it's more like:

// -fdefault-real-8 alone promotes the width for DOUBLE PRECISION to 16 bytes. -fdefault-double-8 overwrites that and sets the width back to 8 bytes.

Since this patch adds gfortran style semantics, it would be worth adding a link to the docs somewhere at the top.

This revision now requires changes to proceed.Feb 17 2021, 8:06 AM
arnamoy10 added inline comments.Feb 17 2021, 8:15 AM
flang/lib/Frontend/CompilerInvocation.cpp
318

I thought it is not necessary because doublePrecisionKind is automatically initialized to double the size of defaultRealKind_ in here--> int doublePrecisionKind_{2 * defaultRealKind_};.

333–335

Sure, will do!

awarzynski added inline comments.Feb 17 2021, 8:34 AM
flang/lib/Frontend/CompilerInvocation.cpp
318

Yes, but the default value for defaultRealKind_ is 4 and here you are setting it to 8. So, correct me if I'm wrong, but when the driver is here, the following has happened:

int defaultIntegerKind_{4};
int defaultRealKind_{defaultIntegerKind_};
int doublePrecisionKind_{2 * defaultRealKind_};

So dublePrecisionKind_ is 8 rather than 16.

arnamoy10 updated this revision to Diff 324334.Feb 17 2021, 9:05 AM
  • Added option for setting DOUBLE PRECISION when fdefault-real-8 is given
  • Also added link to relevant gfortran doc
tskeith added inline comments.Feb 17 2021, 10:11 AM
flang/lib/Frontend/CompilerInvocation.cpp
318

You can test these are working correctly by compiling a module like this:

module m
  implicit none
  real :: x
  double precision :: y
  integer, parameter :: real_kind = kind(x)
  integer, parameter :: double_kind = kind(y)
end

Then check for double_kind=8_4 (or whatever) in the generated .mod file.

For -flarge-sizes:

real :: z(10)
integer, parameter :: size_kind = kind(ubound(z, 1))
arnamoy10 updated this revision to Diff 324673.Feb 18 2021, 9:14 AM

Updated test case to check module content

awarzynski added inline comments.Feb 18 2021, 10:21 AM
flang/test/Flang-Driver/fdefault.f90
2

I think that this test is a great improvement compared to what we had before. Thank you @tskeith for the suggestion and @arnamoy10 for implementing it!

A have a few additional ideas how to strengthen this:

  1. It would be great to see FileCheck instead of grep (with FileCheck you can check more lines easily)
  1. One RUN line per flag. When we mix flags (e.g. -fdefault-real-8 -flarge-sizes), it's not clear whether we are testing one particular flag (e.g. -flarge-sizes) or how various flags interact with each other (e.g. -fdefault-real-8 and -flarge-sizes).

What about:

  • 1 RUN line without any flags + FileCheck checks for both REAL and DOUBLE
  • 1 RUN line with -fdefault-real-8 + FileCheck` checks for both REAL and DOUBLE
  • 1 RUN line with -fdefault-double-8 -fdefault-real-8 + FileCheck checks for both REAL and DOUBLE
  • 1 RUN line with -fdefault-double-8 + FileCheck check to demonstrates that this is an error
  1. Separate file for -flarge-sizes. Otherwise there's a lot being tested here. No need to mix with other flags.
59

FIXME

tskeith added inline comments.Feb 18 2021, 10:55 AM
flang/test/Flang-Driver/fdefault.f90
5

Can't this work with the f18 driver too? That's the best way to ensure they are consistent.

11

I don't think you need to create a subdirectory. %t is a temp directory specific to this test.

So I'm suggesting:

! RUN: %flang -fsyntax-only -fdefault-real-8 %s
awarzynski added inline comments.Feb 18 2021, 11:12 AM
flang/test/Flang-Driver/fdefault.f90
11

From LIT docs:

%t	temporary file name unique to the test
%T	parent directory of %t (not unique, deprecated, do not use)

So, IIUC, we do need to create a directory. We could skip <dir-flang-new> in the directory name, but it does no harm and can be really helpful when parsing CI logs.

I might be missing something though. @tskeith ?

tskeith added inline comments.Feb 18 2021, 12:03 PM
flang/test/Flang-Driver/fdefault.f90
11

I thought lit created an empty directory for %t but apparently not. You just get whatever was left over from last run. So the safest thing to do is this for each compilation command:
! RUN: rm -rf %t && mkdir %t && %flang -module-dir %t ...

It's true that adding an extra subdirectory does no harm besides clutter, but what's the benefit?

arnamoy10 updated this revision to Diff 324785.Feb 18 2021, 3:04 PM
arnamoy10 marked an inline comment as not done.
  • Rewritten the test cases
  • Moved flarge-sizes to a standalone test file
jansvoboda11 resigned from this revision.Feb 19 2021, 5:11 AM
awarzynski accepted this revision.Feb 19 2021, 6:20 AM

I've left a few comments, but these are nice-to-haves from my perspective. @tskeith , what do you think about the suggested changes in f18? We could upload a separate patch for that.

flang/test/Flang-Driver/fdefault.f90
5

I think that this is a good idea, but there are two things that need to be addressed first:

OPTION NAMES

f18 uses -module rather than -module-dir. I suggest adding an alias, -module-dir, to f18 (e.g. similarly to how -fparse-only and -fsyntax-only are handled)

IMPLEMENTATION
If I understand correctly, the current implementation of -fdefault-real-8 in f18 is incomplete, see here:

} else if (arg == "-r8" || arg == "-fdefault-real-8") {
  defaultKinds.set_defaultRealKind(8);

I think that there should be defaultKinds.set_defaultRealKind(16); as well.

@tskeith could you confirm?

11

%t is normally used as a temporary file name. By using e.g. %t/dir-flang-new instead, we communicate it clearly that we are using it as a directory name. This is obvious now (and may seem unnecessary), but it might be less so in 6 months or when somebody works on this project (or particular test) in the future.

Either way, this test is functionally correct and in fact incredibly helpful. Both %t and %t/dir-flang-new will work perfectly fine.

flang/test/Flang-Driver/flarge_sizes.f90
2

Could you add a NOOPTION variant like you did in fdefault.f90?

This revision is now accepted and ready to land.Feb 19 2021, 6:20 AM
tskeith added inline comments.Feb 19 2021, 7:21 AM
flang/test/Flang-Driver/fdefault.f90
5

I think that this is a good idea, but there are two things that need to be addressed first:

OPTION NAMES

f18 uses -module rather than -module-dir. I suggest adding an alias, -module-dir, to f18 (e.g. similarly to how -fparse-only and -fsyntax-only are handled)

Yes, in general when an option is given a different name in the new driver that option should be added to f18. Not only for testing consistent behavior but also because f18 gets more usage so any problems are more likely to turn up.

IMPLEMENTATION
If I understand correctly, the current implementation of -fdefault-real-8 in f18 is incomplete, see here:

} else if (arg == "-r8" || arg == "-fdefault-real-8") {
  defaultKinds.set_defaultRealKind(8);

I think that there should be defaultKinds.set_defaultRealKind(16); as well.

Do you mean set_doublePrecisionKind(16)? Yes, if that's how -fdefault-real-8 is supposed to behave, it should work that way in f18 too.

arnamoy10 updated this revision to Diff 324997.Feb 19 2021, 8:42 AM

Added a missing option test for -flarge-sizes

flang/test/Flang-Driver/flarge_sizes.f90
2

Done!

arnamoy10 updated this revision to Diff 327572.Mar 2 2021, 1:50 PM

Rebased on main

awarzynski requested changes to this revision.Mar 3 2021, 6:18 AM

This patch won't build with BUILD_SHARED_LIBS set to On. That's because a new dependency in CompilerInvocation is introduced (on IntrinsicTypeDefaultKinds). This dependency is currently not satisfied in the unit tests. This should fix it:

diff --git a/flang/unittests/Frontend/CMakeLists.txt b/flang/unittests/Frontend/CMakeLists.txt
index 88ec86f49291..a288aa0c99c9 100644
--- a/flang/unittests/Frontend/CMakeLists.txt
+++ b/flang/unittests/Frontend/CMakeLists.txt
@@ -10,4 +10,5 @@ target_link_libraries(FlangFrontendTests
   flangFrontendTool
   FortranParser
   FortranSemantics
+  FortranCommon
 )

I've left a few other comments inline.

clang/include/clang/Driver/Options.td
4307–4314

[nit] I believe that you copied the long version from here. I would instead use the short versions from this:

gcc --help=fortran | grep default
 -Wreturn-type               Warn whenever a function's return type defaults to "int" (C), or about inconsistent return types (C++).
 -fdefault-double-8          Set the default double precision kind to an 8 byte wide type.
 -fdefault-integer-8         Set the default integer kind to an 8 byte wide type.
 -fdefault-real-8            Set the default real kind to an 8 byte wide type.
 -fmodule-private            Set default accessibility of module entities to PRIVATE.
 -fopenacc-dim=              Specify default OpenACC compute dimensions.
flang/test/Flang-Driver/fdefault.f90
54

Fails for f18 as it does not print error here.

flang/test/Flang-Driver/pipeline.f90
1 ↗(On Diff #327572)

This test will fail with FLANG_BUILD_NEW_DRIVER set to Off (because it should not be run with f18). If you add ! REQUIRES: new-flang-driver then everything will be fine.

Instead of adding that ^^^, could you move it to frontend-forwarding.f90?

This revision now requires changes to proceed.Mar 3 2021, 6:18 AM
arnamoy10 updated this revision to Diff 327789.Mar 3 2021, 7:17 AM

Fixing build failures, also moved the pipeline check to an existing file.

awarzynski accepted this revision.Mar 3 2021, 9:51 AM

I tested this with FLANG_BUILD_NEW_DRIVER set to`On` and Off - all works fine. It's been a tricky patch @arnamoy10 , thank you for all the effort!

I believe that you've addressed all of @tskeith comments too. In my opinion this is ready to be merged (though I'd wait 1 day just in case).

This revision is now accepted and ready to land.Mar 3 2021, 9:51 AM
This revision was landed with ongoing or failed builds.Mar 4 2021, 5:30 AM
This revision was automatically updated to reflect the committed changes.