This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa784de783af5: [flang] Add -ffp-contract option processing
(authored by tblah, committed by kiranchandramohan).
Repository:
rG LLVM Github Monorepo
tblah updated this revision to Diff 471958.
tblah added a comment.
I've updated the patch with @awarzynski's nit and an up-to-date context.
@kiranchandramohan please could you commit for me
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136080/new/
https://reviews.llvm.org/D136080
File
awarzynski accepted this revision.
awarzynski added a comment.
LGTM, thanks for implementing this!
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:98-99
+} else
+ // Clang's "fast-honor-pragmas" option is not supported because it is
+ // non-standard and pragmas
tblah updated this revision to Diff 471163.
tblah edited the summary of this revision.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136080/new/
https://reviews.llvm.org/D136080
Files:
clang/include/clang/Basic/DiagnosticDriverKinds.td
clang/include/clang/Driver/Options.td
clang/li
MaskRay added inline comments.
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:89
+const StringRef Val = A->getValue();
+if (Val.equals("fast") || Val.equals("off")) {
+ FPContract = Val;
We prefer `==` to `equals`
CHANGES SINCE LAST ACTION
htt
awarzynski added a comment.
Thanks for all the updates, Tom! I have a few more suggestions.
From the summary:
> implement these pragmas
Could you explain what pragmas you are referring to here? (i.e. Clang pragmas
for C and C++ + link)
> gfortran uses "fast" by default
For our future self, c
awarzynski added a comment.
Added Clang reviewers who touched the definition of `--fp-contract` most
recently. Mostly for visibility, but lets give them at least a couple of days
to take a look at the changes in Options.td.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136080/new/
htt
vzakhari accepted this revision.
vzakhari added a comment.
This revision is now accepted and ready to land.
Thank you! LGTM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136080/new/
https://reviews.llvm.org/D136080
___
cfe-commits mailing lis
tblah updated this revision to Diff 470496.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136080/new/
https://reviews.llvm.org/D136080
Files:
clang/include/clang/Basic/DiagnosticDriverKinds.td
clang/include/clang/Driver/Options.td
clang/lib/Driver/ToolChains/Flang.cpp
flang/inclu
vzakhari added inline comments.
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:94
+ << Val << A->getOption().getName() << "fast";
+ FPContract = "fast";
+} else
I know I suggested myself mapping `on` to `fast`, but it seems it will be more
r
tblah updated this revision to Diff 470184.
tblah marked 2 inline comments as not done.
tblah edited the summary of this revision.
tblah added a comment.
Updated diff with a further reduced help text after discussions with @awarzynski
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136080/
awarzynski added inline comments.
Comment at: clang/include/clang/Driver/Options.td:1926
+ " Default is 'fast' for CUDA, 'fast-honor-pragmas' for HIP, 'off' for flang,
and 'on' otherwise.">,
+ HelpText<"Form fused FP ops (e.g. FMAs): fast | off (clang, flang), on |
fast-honor
tblah updated this revision to Diff 469556.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136080/new/
https://reviews.llvm.org/D136080
Files:
clang/include/clang/Basic/DiagnosticDriverKinds.td
clang/include/clang/Driver/Options.td
clang/lib/Driver/ToolChains/Flang.cpp
flang/inclu
tblah added inline comments.
Comment at: flang/include/flang/Frontend/LangOptions.h:29
+
+// Enable the floating point pragma
+FPM_On,
vzakhari wrote:
> tblah wrote:
> > vzakhari wrote:
> > > tblah wrote:
> > > > awarzynski wrote:
> > > > > What are these
vzakhari added inline comments.
Comment at: flang/include/flang/Frontend/LangOptions.h:29
+
+// Enable the floating point pragma
+FPM_On,
tblah wrote:
> vzakhari wrote:
> > tblah wrote:
> > > awarzynski wrote:
> > > > What are these pragmas? Perhaps you c
awarzynski added inline comments.
Comment at: flang/test/Driver/driver-help-hidden.f90:34
! CHECK-NEXT: Use as character line width in fixed mode
+! CHECK-NEXT: -ffp-contract= Form fused FP ops (e.g. FMAs): fast (fuses
across statements disregarding pragmas) | on (only fuses i
tblah added inline comments.
Comment at: flang/include/flang/Frontend/LangOptions.h:29
+
+// Enable the floating point pragma
+FPM_On,
vzakhari wrote:
> tblah wrote:
> > awarzynski wrote:
> > > What are these pragmas? Perhaps you can add a test that would
vzakhari added inline comments.
Comment at: flang/include/flang/Frontend/LangOptions.h:29
+
+// Enable the floating point pragma
+FPM_On,
tblah wrote:
> awarzynski wrote:
> > What are these pragmas? Perhaps you can add a test that would include them?
> I
tblah updated this revision to Diff 468840.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136080/new/
https://reviews.llvm.org/D136080
Files:
clang/include/clang/Driver/Options.td
clang/lib/Driver/ToolChains/Flang.cpp
flang/include/flang/Frontend/CompilerInvocation.h
flang/includ
tblah marked 8 inline comments as done.
tblah added a comment.
>> The omission of the fast-honor-pragmas argument from the compiler driver is
>> deliberate.
>
> Where is it omitted?
This argument is only supported in the frontend driver, not the compiler driver:
flang-new -ffp-contract=fast-h
DavidTruby added a comment.
Glad to see this flag being added to `flang-new`! As a note to self I've
written some tests that should be updated once this lands that currently don't
pass through the `flang-new` driver.
Comment at: flang/test/Driver/driver-help.f90:108
! HELP-F
awarzynski added a comment.
> The omission of the fast-honor-pragmas argument from the compiler driver is
> deliberate.
Where is it omitted?
> I suspect the CI failure on windows is unrelated to my code
I agree.
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:165
+ // Fl
tblah added a comment.
Linux CI is passing. I suspect the CI failure on windows is unrelated to my
code: the test failure is for clang-scan-deps and the previous version of the
patch passed CI.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136080/new/
https://reviews.llvm.org/D136080
tblah updated this revision to Diff 468475.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136080/new/
https://reviews.llvm.org/D136080
Files:
clang/include/clang/Driver/Options.td
clang/lib/Driver/ToolChains/Flang.cpp
flang/include/flang/Frontend/CompilerInvocation.h
flang/includ
tblah marked 5 inline comments as done.
tblah added a comment.
Thanks for taking a look. I've updated accordingly and will upload the patch
soon.
> are you confident that we will need LangOptions.def?
Clang places this flag (and many other floating point options) in LangOptions
so I thought I
awarzynski added a comment.
Thanks for implementing this, @tblah!
Two high level questions/requests:
- are you confident that we will need LangOptions.def?
- can you upload a patch with full context? (some details can be found here:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via
tblah updated this revision to Diff 468258.
tblah added a comment.
clang-format plus cleaning up typos.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136080/new/
https://reviews.llvm.org/D136080
Files:
clang/include/clang/Driver/Options.td
clang/lib/Driver/ToolChains/Flang.cpp
fl
tblah created this revision.
tblah added reviewers: kiranchandramohan, MatsPetersson, DavidTruby, rovka,
vzakhari.
tblah added a project: Flang.
Herald added a subscriber: jdoerfert.
Herald added a reviewer: sscalpone.
Herald added a reviewer: awarzynski.
Herald added a project: All.
tblah request
28 matches
Mail list logo
