CarolineConcatto added a comment.
I'll close this PR as we believe we will not need this flag for Flang driver.
There are some development happening in:
https://github.com/banach-space/llvm-project
for now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
CarolineConcatto abandoned this revision.
CarolineConcatto added a comment.
I'll close this PR as we believe we will not need this flag for Flang driver.
There are some development happening in:
https://github.com/banach-space/llvm-project
for now.
Repository:
rG LLVM Github Monorepo
CHANGES
CarolineConcatto updated this revision to Diff 261806.
CarolineConcatto marked 7 inline comments as done.
CarolineConcatto added a comment.
Rename the flag and fix reviews comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73951/new/
awarzynski added inline comments.
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:71
const auto& D = C.getDriver();
- const std::string = D.getFFCGenericFortranName();
+ const std::string = D.getGenericFortranFE();
const char *FortranName;
[nit]
awarzynski added a comment.
@CarolineConcatto thank you for the updates!
- It looks like you accidentally pushed two commits in one patch :) Could you
please squash and resubmit? (this should clean-up the diff). Ta!
- Perhaps `CFCGenericFlangName` instead of `GenericFortranFE`? This would
CarolineConcatto added inline comments.
Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:10
+! RUN: cp %clang %t1
+! RUN: %clang --driver-mode=flang -fortran-fe %basename_t.tmp1 -### %s 2>&1 |
FileCheck --check-prefixes=ALL %s
+
awarzynski
CarolineConcatto updated this revision to Diff 261451.
CarolineConcatto marked 3 inline comments as done.
CarolineConcatto added a comment.
This patch updates the flag name from ffc-fortran-name to cfc-fortran-name,
where cfc stands for custom frontend compiler.
Moreover, the tests need pruning
awarzynski added a comment.
@richard.barton.arm @CarolineConcatto thank you both for clarification!
@CarolineConcatto , if you prefer to go ahead with this approach - could you
please address all the outstanding comments? I've left a few comments that I'd
like addressed first, but nothing
richard.barton.arm added a comment.
So, coming back to this after a few weeks break.
I agree with the point @awarzynski makes on this option being powerful and I
think your alternative suggestion of using a different hard-coded name could
work well at this stage of the project. I think the
CarolineConcatto added a comment.
@awarzynski If we don't foresee a use for this flag beside the problem I've
faced, then I am fine to remove and only replace by flang-tmp.
But as we discuss this flag enables flang driver to call another driver,
basically cross-compilation.
That could be a
awarzynski added a comment.
The suggested flag, `-fortran-fe`, feels quite powerful and I wonder whether
it's required this early into the development of the Flang driver?
IIUC, this flag would be used to workaround the fact that currently `flang` is
a bash script and hence can't be used when
sscalpone added inline comments.
Comment at: clang/test/Driver/flang/flang-driver-2-frontend01.f90:1
+! Check wich name of flang frontend is invoked by the driver
+
typo //wich//
Comment at:
sscalpone added a comment.
type in the commit message: //esle// the frontend name is hard-coded to
"flang", use that to
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73951/new/
https://reviews.llvm.org/D73951
richard.barton.arm added inline comments.
Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:10
+! RUN: cp %clang %t1
+! RUN: %clang --driver-mode=flang -fortran-fe %basename_t.tmp1 -### %s 2>&1 |
FileCheck --check-prefixes=ALL %s
+
CarolineConcatto marked an inline comment as done.
CarolineConcatto added inline comments.
Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:10
+! RUN: cp %clang %t1
+! RUN: %clang --driver-mode=flang -fortran-fe %basename_t.tmp1 -### %s 2>&1 |
FileCheck
richard.barton.arm added inline comments.
Comment at: clang/include/clang/Driver/Options.td:268
+def fortran_fe : Separate<["-"], "fortran-fe">, InternalDriverOpt,
+ HelpText<"Name for native Fortran compiler">;
richard.barton.arm wrote:
> This is not right.
richard.barton.arm added a comment.
Still not sure why all the tests are needed here. The first one looks fine,
i.e. we test that --fortran-fe=
calls to that copy. The second one appears to test the default behaviour with
no option, but why does it do it via a different name for clang, why
CarolineConcatto updated this revision to Diff 244635.
CarolineConcatto added a comment.
- [Clang]Rename flag and update the tests
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73951/new/
https://reviews.llvm.org/D73951
Files:
CarolineConcatto added a comment.
@richard.barton.arm A new patch will land today with the changes asked by you.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73951/new/
https://reviews.llvm.org/D73951
richard.barton.arm added a comment.
I'd still like to see the nits addressed and comments on the tests addressed
before approving.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73951/new/
https://reviews.llvm.org/D73951
DavidTruby accepted this revision.
DavidTruby marked an inline comment as done.
DavidTruby added a comment.
LGTM but wait for someone else to approve
Comment at: clang/lib/Driver/Driver.cpp:131
CCLogDiagnostics(false), CCGenDiagnostics(false),
-
richard.barton.arm added inline comments.
Comment at: clang/include/clang/Driver/Options.td:264
MetaVarName<"">;
+def fcc_fortran_name : Separate<["-"], "ffc-fortran-name">, InternalDriverOpt,
+ HelpText<"Name for native Fortran compiler">;
CarolineConcatto
CarolineConcatto marked 5 inline comments as done.
CarolineConcatto added inline comments.
Comment at: clang/include/clang/Driver/Options.td:264
MetaVarName<"">;
+def fcc_fortran_name : Separate<["-"], "ffc-fortran-name">, InternalDriverOpt,
+ HelpText<"Name for native
richard.barton.arm requested changes to this revision.
richard.barton.arm added a comment.
A few comments running through that need addressing.
In addition - have you checked the behaviour of this option with `-Bprefix`?
Looking at the code for Driver.GetProgramPath, it seems like that would
DavidTruby requested changes to this revision.
DavidTruby added a comment.
This revision now requires changes to proceed.
If what I've suggested above doesn't work then the patch lgtm as is
Comment at: clang/lib/Driver/Driver.cpp:131
CCLogDiagnostics(false),
25 matches
Mail list logo
