[PATCH] D96344: [flang][driver] Add options for -fdefault* and -flarge-sizes

Andrzej Warzynski via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Feb 19 06:20:45 PST 2021


awarzynski accepted this revision.
awarzynski added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: flang/test/Flang-Driver/fdefault.f90:4
+
+! REQUIRES: new-flang-driver
+
----------------
tskeith wrote:
> Can't this work with the f18 driver too? That's the best way to ensure they are consistent.
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 [[ https://github.com/llvm/llvm-project/blob/b6db47d7e044730dc3c9b35dae6697eee0885dbf/flang/tools/f18/f18.cpp#L568-L569 | 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?


================
Comment at: flang/test/Flang-Driver/fdefault.f90:10
+! RUN: not %flang-new -fsyntax-only -fdefault-double-8 %s  2>&1 | FileCheck %s --check-prefix=DOUBLE
+! RUN: mkdir -p %t/dir-flang-new && %flang-new -fsyntax-only -module-dir %t/dir-flang-new -fdefault-real-8 %s  2>&1
+! RUN: cat %t/dir-flang-new/m.mod | grep 'real_kind=8_4'
----------------
tskeith wrote:
> awarzynski wrote:
> > tskeith wrote:
> > > 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
> > > ```
> > From [[ https://llvm.org/docs/CommandGuide/lit.html#substitutions | 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 ? 
> 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?
`%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.


================
Comment at: flang/test/Flang-Driver/flarge_sizes.f90:1
+! Ensure argument -flarge-sizes works as expected.
+! TODO: Add checks when actual codegen is possible for this family
----------------
Could you add a `NOOPTION` variant like you did in fdefault.f90?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96344/new/

https://reviews.llvm.org/D96344



More information about the cfe-commits mailing list