[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 3 inline comments as done.
mibintc added inline comments.



Comment at: clang/docs/UsersManual.rst:1305
+   and ``noexcept``. Note that -fp-model=[no]except can be combined with the
+   other three settings for this option. Details:
+

andrew.w.kaylor wrote:
> rjmccall wrote:
> > mibintc wrote:
> > > rjmccall wrote:
> > > > Combined how?  With a comma?
> > > > 
> > > > This option seems to have two independent dimensions.  Is that 
> > > > necessary for command-line compatibility with ICC, or can we separate 
> > > > it into two options?
> > > > 
> > > > The documentation should mention the default behavior along both 
> > > > dimensions.  Is it possible to override a prior instance of this option 
> > > > to get this default behavior back?
> > > > 
> > > > You mention that this `-fp-model=fast` is equivalent to `-ffast-math`.  
> > > > How does this option interact with that one if both are given on a 
> > > > command line?
> > > > 
> > > > Please put option text in backticks wherever it appears.
> > > > 
> > > > Most of these comments apply to `-fp-speculation` as well.
> > > > Combined how? With a comma?
> > > 
> > > > This option seems to have two independent dimensions. Is that necessary 
> > > > for command-line compatibility with ICC, or can we separate it into two 
> > > > options?
> > > Yes that's right, there are 2 dimensions.  I wrote it like this for 
> > > identical compatibility with icc, and cl.exe also defines the option this 
> > > way, to specify multiple values simultaneously. However I think it would 
> > > be reasonable and good to split them into separate options.  I will 
> > > discuss this with the folks back home.
> > > 
> > > > The documentation should mention the default behavior along both 
> > > > dimensions. 
> > > I added this info into the doc
> > > 
> > > > Is it possible to override a prior instance of this option to get this 
> > > > default behavior back?
> > > The 3 values along one dimension, precise, strict, fast if they appear 
> > > multiple times in the command line, the last value will be the setting 
> > > along that dimension.  Ditto with the other dimension, the rightmost 
> > > occurrence of except or noexcept will be the setting. 
> > > 
> > > > You mention that this -fp-model=fast is equivalent to -ffast-math. How 
> > > > does this option interact with that one if both are given on a command 
> > > > line?
> > > The idea is that they are synonyms so if either or both appeared on the 
> > > command line, the effect would be identical. 
> > > 
> > > I'll upload another patch with a few documentation updates and get back 
> > > to you about splitting the fp-model option into multiple options.  
> > > (Longer term, there are 2 other dimensions to fp-model)
> > > 
> > > And thanks for the review
> > > Yes that's right, there are 2 dimensions. I wrote it like this for 
> > > identical compatibility with icc, and cl.exe also defines the option this 
> > > way, to specify multiple values simultaneously. However I think it would 
> > > be reasonable and good to split them into separate options. I will 
> > > discuss this with the folks back home.
> > 
> > Okay.  There's certainly some value in imitating existing compilers, but it 
> > sounds like a lot has been forced into one option, so maybe we should take 
> > the opportunity to split it up.  If we do split it, though, I think the 
> > different dimensions should have different base spellings, rather than 
> > being repeated uses of `-fp-model`.
> > 
> > > The 3 values along one dimension, precise, strict, fast if they appear 
> > > multiple times in the command line, the last value will be the setting 
> > > along that dimension.
> > 
> > Okay.  This wasn't clear to me from the code, since the code also has an 
> > "off" option.
> > 
> > > The idea is that they are synonyms so if either or both appeared on the 
> > > command line, the effect would be identical.
> > 
> > Right, but compiler options are allowed to conflict with each other, with 
> > the general rule being that the last option "wins".  So what I'm asking is 
> > if that works correctly with this option and `-ffast-math`, so that e.g. 
> > `-ffast-math -fp-model=strict` leaves you with strict FP but 
> > `-fp-model=strict -ffast-math` leaves you with fast FP.  (That is another 
> > reason why it's best to have one aspect settled in each option: because you 
> > don't have to merge information from different uses of the option.)
> > 
> > At any rate, the documentation should be clear about how this interacts 
> > with `-ffast-math`.  You might even consider merging this into the 
> > documentation for `-ffast-math`, or at least revising that option's 
> > documentation.  Does `-fp-model=fast` cause `__FAST_MATH__` to be defined?
> > 
> > Also, strictly speaking, this should be `-ffp-model`, right?
> I think the ICC interface includes the exception option for 
> compatibility/consistency with 

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added inline comments.



Comment at: clang/docs/UsersManual.rst:1309
+
+   * ``precise`` Disables optimizations that are not value-safe on 
floating-point data, although FP contraction (FMA) is enabled.
+   * ``strict`` Enables precise and except, and disables contractions (FMA).

There's a bit of ambiguity here because FP contraction isn't an on/off switch 
in LLVM. It has three settings: on, off, and fast. What you've done in this 
patch sets it to 'on' for precise, 'off' for strict, and 'fast' for fast. That 
sounds reasonable, but it's not what ICC and MSVC do. ICC and MSVC both have a 
behavior equivalent to -ffp-contract=fast in the precise model.

The idea behind this is that FMA operations are actually more precise than the 
non-contracted operations. They don't always give the same result, but they 
give a more precise result. The problem with this is that if we adopt this 
approach it leaves us with no fp model that corresponds to the default compiler 
behavior if you don't specify an -fp-model  at all.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added inline comments.



Comment at: clang/docs/UsersManual.rst:1305
+   and ``noexcept``. Note that -fp-model=[no]except can be combined with the
+   other three settings for this option. Details:
+

rjmccall wrote:
> mibintc wrote:
> > rjmccall wrote:
> > > Combined how?  With a comma?
> > > 
> > > This option seems to have two independent dimensions.  Is that necessary 
> > > for command-line compatibility with ICC, or can we separate it into two 
> > > options?
> > > 
> > > The documentation should mention the default behavior along both 
> > > dimensions.  Is it possible to override a prior instance of this option 
> > > to get this default behavior back?
> > > 
> > > You mention that this `-fp-model=fast` is equivalent to `-ffast-math`.  
> > > How does this option interact with that one if both are given on a 
> > > command line?
> > > 
> > > Please put option text in backticks wherever it appears.
> > > 
> > > Most of these comments apply to `-fp-speculation` as well.
> > > Combined how? With a comma?
> > 
> > > This option seems to have two independent dimensions. Is that necessary 
> > > for command-line compatibility with ICC, or can we separate it into two 
> > > options?
> > Yes that's right, there are 2 dimensions.  I wrote it like this for 
> > identical compatibility with icc, and cl.exe also defines the option this 
> > way, to specify multiple values simultaneously. However I think it would be 
> > reasonable and good to split them into separate options.  I will discuss 
> > this with the folks back home.
> > 
> > > The documentation should mention the default behavior along both 
> > > dimensions. 
> > I added this info into the doc
> > 
> > > Is it possible to override a prior instance of this option to get this 
> > > default behavior back?
> > The 3 values along one dimension, precise, strict, fast if they appear 
> > multiple times in the command line, the last value will be the setting 
> > along that dimension.  Ditto with the other dimension, the rightmost 
> > occurrence of except or noexcept will be the setting. 
> > 
> > > You mention that this -fp-model=fast is equivalent to -ffast-math. How 
> > > does this option interact with that one if both are given on a command 
> > > line?
> > The idea is that they are synonyms so if either or both appeared on the 
> > command line, the effect would be identical. 
> > 
> > I'll upload another patch with a few documentation updates and get back to 
> > you about splitting the fp-model option into multiple options.  (Longer 
> > term, there are 2 other dimensions to fp-model)
> > 
> > And thanks for the review
> > Yes that's right, there are 2 dimensions. I wrote it like this for 
> > identical compatibility with icc, and cl.exe also defines the option this 
> > way, to specify multiple values simultaneously. However I think it would be 
> > reasonable and good to split them into separate options. I will discuss 
> > this with the folks back home.
> 
> Okay.  There's certainly some value in imitating existing compilers, but it 
> sounds like a lot has been forced into one option, so maybe we should take 
> the opportunity to split it up.  If we do split it, though, I think the 
> different dimensions should have different base spellings, rather than being 
> repeated uses of `-fp-model`.
> 
> > The 3 values along one dimension, precise, strict, fast if they appear 
> > multiple times in the command line, the last value will be the setting 
> > along that dimension.
> 
> Okay.  This wasn't clear to me from the code, since the code also has an 
> "off" option.
> 
> > The idea is that they are synonyms so if either or both appeared on the 
> > command line, the effect would be identical.
> 
> Right, but compiler options are allowed to conflict with each other, with the 
> general rule being that the last option "wins".  So what I'm asking is if 
> that works correctly with this option and `-ffast-math`, so that e.g. 
> `-ffast-math -fp-model=strict` leaves you with strict FP but 
> `-fp-model=strict -ffast-math` leaves you with fast FP.  (That is another 
> reason why it's best to have one aspect settled in each option: because you 
> don't have to merge information from different uses of the option.)
> 
> At any rate, the documentation should be clear about how this interacts with 
> `-ffast-math`.  You might even consider merging this into the documentation 
> for `-ffast-math`, or at least revising that option's documentation.  Does 
> `-fp-model=fast` cause `__FAST_MATH__` to be defined?
> 
> Also, strictly speaking, this should be `-ffp-model`, right?
I think the ICC interface includes the exception option for 
compatibility/consistency with Microsoft's  /fp option. We can handle that in 
clang-cl. So, I agree that it makes sense to split that out in clang.

ICC's implementation of this actually has four dimensions, only two of which 
are being taken on here. Frankly, I think it's a bit 

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/docs/UsersManual.rst:1305
+   and ``noexcept``. Note that -fp-model=[no]except can be combined with the
+   other three settings for this option. Details:
+

mibintc wrote:
> rjmccall wrote:
> > Combined how?  With a comma?
> > 
> > This option seems to have two independent dimensions.  Is that necessary 
> > for command-line compatibility with ICC, or can we separate it into two 
> > options?
> > 
> > The documentation should mention the default behavior along both 
> > dimensions.  Is it possible to override a prior instance of this option to 
> > get this default behavior back?
> > 
> > You mention that this `-fp-model=fast` is equivalent to `-ffast-math`.  How 
> > does this option interact with that one if both are given on a command line?
> > 
> > Please put option text in backticks wherever it appears.
> > 
> > Most of these comments apply to `-fp-speculation` as well.
> > Combined how? With a comma?
> 
> > This option seems to have two independent dimensions. Is that necessary for 
> > command-line compatibility with ICC, or can we separate it into two options?
> Yes that's right, there are 2 dimensions.  I wrote it like this for identical 
> compatibility with icc, and cl.exe also defines the option this way, to 
> specify multiple values simultaneously. However I think it would be 
> reasonable and good to split them into separate options.  I will discuss this 
> with the folks back home.
> 
> > The documentation should mention the default behavior along both 
> > dimensions. 
> I added this info into the doc
> 
> > Is it possible to override a prior instance of this option to get this 
> > default behavior back?
> The 3 values along one dimension, precise, strict, fast if they appear 
> multiple times in the command line, the last value will be the setting along 
> that dimension.  Ditto with the other dimension, the rightmost occurrence of 
> except or noexcept will be the setting. 
> 
> > You mention that this -fp-model=fast is equivalent to -ffast-math. How does 
> > this option interact with that one if both are given on a command line?
> The idea is that they are synonyms so if either or both appeared on the 
> command line, the effect would be identical. 
> 
> I'll upload another patch with a few documentation updates and get back to 
> you about splitting the fp-model option into multiple options.  (Longer term, 
> there are 2 other dimensions to fp-model)
> 
> And thanks for the review
> Yes that's right, there are 2 dimensions. I wrote it like this for identical 
> compatibility with icc, and cl.exe also defines the option this way, to 
> specify multiple values simultaneously. However I think it would be 
> reasonable and good to split them into separate options. I will discuss this 
> with the folks back home.

Okay.  There's certainly some value in imitating existing compilers, but it 
sounds like a lot has been forced into one option, so maybe we should take the 
opportunity to split it up.  If we do split it, though, I think the different 
dimensions should have different base spellings, rather than being repeated 
uses of `-fp-model`.

> The 3 values along one dimension, precise, strict, fast if they appear 
> multiple times in the command line, the last value will be the setting along 
> that dimension.

Okay.  This wasn't clear to me from the code, since the code also has an "off" 
option.

> The idea is that they are synonyms so if either or both appeared on the 
> command line, the effect would be identical.

Right, but compiler options are allowed to conflict with each other, with the 
general rule being that the last option "wins".  So what I'm asking is if that 
works correctly with this option and `-ffast-math`, so that e.g. `-ffast-math 
-fp-model=strict` leaves you with strict FP but `-fp-model=strict -ffast-math` 
leaves you with fast FP.  (That is another reason why it's best to have one 
aspect settled in each option: because you don't have to merge information from 
different uses of the option.)

At any rate, the documentation should be clear about how this interacts with 
`-ffast-math`.  You might even consider merging this into the documentation for 
`-ffast-math`, or at least revising that option's documentation.  Does 
`-fp-model=fast` cause `__FAST_MATH__` to be defined?

Also, strictly speaking, this should be `-ffp-model`, right?



Comment at: clang/docs/UsersManual.rst:1324
+   * ``strict`` Disables speculation on floating-point operations.
+   * ``safe`` Disables speculation if there is a possibility that speculation 
may cause a floating-point exception.
+

These are exclusive, right?  So the documentation should be ``, not 
``.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc updated this revision to Diff 217426.
mibintc added a comment.

I made a couple changes to the UserManual in response to @rjmccall review


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/fpconstrained.c
  clang/test/Driver/clang_f_opts.c

Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -217,6 +217,11 @@
 // RUN: %clang -### -S -fexec-charset=iso-8859-1 -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-INPUT-CHARSET %s
 // CHECK-INVALID-INPUT-CHARSET: error: invalid value 'iso-8859-1' in '-fexec-charset=iso-8859-1'
 
+// RUN: %clang -### -S -fp-model=fast -fp-model=except -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-FAST-EXCEPT %s
+// RUN: %clang -### -S -fp-model=fast -fp-model=except -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-FAST-EXCEPT %s
+// CHECK-INVALID-FAST-EXCEPT: error: invalid argument 'fp-model=fast' not allowed with 'fp-model=except'
+//
+
 // Test that we don't error on these.
 // RUN: %clang -### -S -Werror\
 // RUN: -falign-functions -falign-functions=2 -fno-align-functions\
Index: clang/test/CodeGen/fpconstrained.c
===
--- /dev/null
+++ clang/test/CodeGen/fpconstrained.c
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fp-model=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=STRICT
+// RUN: %clang_cc1 -fp-model=strict -fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=STRICTEXCEPT
+// RUN: %clang_cc1 -fp-model=strict -no-fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=STRICTNOEXCEPT
+// RUN: %clang_cc1 -fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=EXCEPT
+// RUN: %clang_cc1 -no-fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=NOEXCEPT
+// RUN: %clang_cc1 -fp-model=precise -emit-llvm -o - %s | FileCheck %s -check-prefix=PRECISE
+// RUN: %clang_cc1 -fp-model=fast -emit-llvm -o - %s | FileCheck %s -check-prefix=FAST
+// RUN: %clang_cc1 -fp-model=fast -fp-speculation=fast -emit-llvm -o - %s | FileCheck %s -check-prefix=NOEXCEPT
+// RUN: %clang_cc1 -fp-model=fast -fp-speculation=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=EXCEPT
+// RUN: %clang_cc1 -fp-model=fast -fp-speculation=safe -emit-llvm -o - %s | FileCheck %s -check-prefix=MAYTRAP
+float f0, f1, f2;
+
+void foo(void) {
+  // CHECK-LABEL: define {{.*}}void @foo()
+
+  // MAYTRAP: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.tonearest", metadata !"fpexcept.maytrap")
+  // EXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.tonearest", metadata !"fpexcept.strict")
+  // NOEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.tonearest", metadata !"fpexcept.ignore")
+  // STRICT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.dynamic", metadata !"fpexcept.strict")
+  // STRICTEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.dynamic", metadata !"fpexcept.strict")
+  // STRICTNOEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.dynamic", metadata !"fpexcept.ignore")
+  // PRECISE: fadd float
+  // FAST: fadd fast
+  f0 = f1 + f2;
+
+  // CHECK: ret
+}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3074,6 +3074,63 @@
   Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
   }
 
+  LangOptions::FPModelKind FPM = LangOptions::FPM_Off;
+  if (Arg *A = Args.getLastArg(OPT_fp_model_EQ)) {
+StringRef Val = A->getValue();
+if (Val == "precise")
+  FPM = LangOptions::FPM_Precise;
+else if (Val == "strict")
+  FPM = LangOptions::FPM_Strict;
+else if (Val == "fast")
+  FPM = LangOptions::FPM_Fast;
+else
+  Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
+  }
+  Opts.getFPMOptions().setFPModelSetting(FPM);
+
+  LangOptions::FPModelExceptKind FPME = LangOptions::FPME_Off;
+  if (const Arg *A =
+  Args.getLastArg(OPT_fp_model_except, OPT_no_fp_model_except))
+switch (A->getOption().getID()) {
+case OPT_fp_model_except:
+  FPME = LangOptions::FPME_Except;
+  break;
+case OPT_no_fp_model_except:
+  FPME 

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 2 inline comments as done.
mibintc added inline comments.



Comment at: clang/docs/UsersManual.rst:1305
+   and ``noexcept``. Note that -fp-model=[no]except can be combined with the
+   other three settings for this option. Details:
+

rjmccall wrote:
> Combined how?  With a comma?
> 
> This option seems to have two independent dimensions.  Is that necessary for 
> command-line compatibility with ICC, or can we separate it into two options?
> 
> The documentation should mention the default behavior along both dimensions.  
> Is it possible to override a prior instance of this option to get this 
> default behavior back?
> 
> You mention that this `-fp-model=fast` is equivalent to `-ffast-math`.  How 
> does this option interact with that one if both are given on a command line?
> 
> Please put option text in backticks wherever it appears.
> 
> Most of these comments apply to `-fp-speculation` as well.
> Combined how? With a comma?

> This option seems to have two independent dimensions. Is that necessary for 
> command-line compatibility with ICC, or can we separate it into two options?
Yes that's right, there are 2 dimensions.  I wrote it like this for identical 
compatibility with icc, and cl.exe also defines the option this way, to specify 
multiple values simultaneously. However I think it would be reasonable and good 
to split them into separate options.  I will discuss this with the folks back 
home.

> The documentation should mention the default behavior along both dimensions. 
I added this info into the doc

> Is it possible to override a prior instance of this option to get this 
> default behavior back?
The 3 values along one dimension, precise, strict, fast if they appear multiple 
times in the command line, the last value will be the setting along that 
dimension.  Ditto with the other dimension, the rightmost occurrence of except 
or noexcept will be the setting. 

> You mention that this -fp-model=fast is equivalent to -ffast-math. How does 
> this option interact with that one if both are given on a command line?
The idea is that they are synonyms so if either or both appeared on the command 
line, the effect would be identical. 

I'll upload another patch with a few documentation updates and get back to you 
about splitting the fp-model option into multiple options.  (Longer term, there 
are 2 other dimensions to fp-model)

And thanks for the review


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/docs/UsersManual.rst:1299
 
+.. option:: -fp-model=[values]
+

This should be something like `-fp-model=`.  Square brackets mean 
optional elements in these docs.



Comment at: clang/docs/UsersManual.rst:1305
+   and ``noexcept``. Note that -fp-model=[no]except can be combined with the
+   other three settings for this option. Details:
+

Combined how?  With a comma?

This option seems to have two independent dimensions.  Is that necessary for 
command-line compatibility with ICC, or can we separate it into two options?

The documentation should mention the default behavior along both dimensions.  
Is it possible to override a prior instance of this option to get this default 
behavior back?

You mention that this `-fp-model=fast` is equivalent to `-ffast-math`.  How 
does this option interact with that one if both are given on a command line?

Please put option text in backticks wherever it appears.

Most of these comments apply to `-fp-speculation` as well.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

ping, looking for a code review, especially from front end folks.  thank you!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Kevin P. Neal via Phabricator via cfe-commits]

kpn added a comment.

I don't believe I have any further comments. What do the front-end guys say?




Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:126
+  case LangOptions::FPM_Precise:
+  case LangOptions::FPM_Fast:
+break;

andrew.w.kaylor wrote:
> mibintc wrote:
> > mibintc wrote:
> > > kpn wrote:
> > > > Wait, so "fast" and "precise" are the same thing? That doesn't sound 
> > > > like where the documentation you put in the ticket says "the compiler 
> > > > preserves the source expression ordering and rounding properties of 
> > > > floating-point".
> > > > 
> > > > (Yes, I saw below where "fast" turns on the fast math flags but 
> > > > "precise" doesn't. That doesn't affect my point here.)
> > > "precise" doesn't necessitate the use of Constrained Intrinsics, And 
> > > likewise for "fast".   The words "compiler preserves the source 
> > > expression ordering" were copied from the msdn documentation for 
> > > /fp:precise as you explained it would be useful to have the msdn 
> > > documentation for the option in case it goes offline in, say, 30 years.  
> > > The ICL Intel compiler also provides equivalent floating point options. 
> > > The Intel documentation for precise is phrased differently "Disables 
> > > optimizations that are not value-safe on floating-point data."  
> > > 
> > > fp-model=precise should enable contractions, if that's not true at 
> > > default (I mean, clang -c) then this patch is missing that.
> > > 
> > > fp-model=fast is the same as requesting ffast-math 
> > Well, we haven't heard from Andy yet, but he told me some time ago that 
> > /fp:precise corresponds more or less (there was wiggle room) to clang's 
> > default behavior.  It sounds like you think the description in the msdn of 
> > /fp:precise isn't describing clang's default behavior, @kpn can you say 
> > more about that, and do you think that ConstrainedIntrinsics should be 
> > created to provide the semantics of /fp:precise? 
> "Precise" means that no value unsafe optimizations will be performed. That's 
> what LLVM does by default. As long as no fast math flags are set, we will not 
> perform optimizations that are not value safe.
OK, I stand corrected.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:126
+  case LangOptions::FPM_Precise:
+  case LangOptions::FPM_Fast:
+break;

mibintc wrote:
> mibintc wrote:
> > kpn wrote:
> > > Wait, so "fast" and "precise" are the same thing? That doesn't sound like 
> > > where the documentation you put in the ticket says "the compiler 
> > > preserves the source expression ordering and rounding properties of 
> > > floating-point".
> > > 
> > > (Yes, I saw below where "fast" turns on the fast math flags but "precise" 
> > > doesn't. That doesn't affect my point here.)
> > "precise" doesn't necessitate the use of Constrained Intrinsics, And 
> > likewise for "fast".   The words "compiler preserves the source expression 
> > ordering" were copied from the msdn documentation for /fp:precise as you 
> > explained it would be useful to have the msdn documentation for the option 
> > in case it goes offline in, say, 30 years.  The ICL Intel compiler also 
> > provides equivalent floating point options. The Intel documentation for 
> > precise is phrased differently "Disables optimizations that are not 
> > value-safe on floating-point data."  
> > 
> > fp-model=precise should enable contractions, if that's not true at default 
> > (I mean, clang -c) then this patch is missing that.
> > 
> > fp-model=fast is the same as requesting ffast-math 
> Well, we haven't heard from Andy yet, but he told me some time ago that 
> /fp:precise corresponds more or less (there was wiggle room) to clang's 
> default behavior.  It sounds like you think the description in the msdn of 
> /fp:precise isn't describing clang's default behavior, @kpn can you say more 
> about that, and do you think that ConstrainedIntrinsics should be created to 
> provide the semantics of /fp:precise? 
"Precise" means that no value unsafe optimizations will be performed. That's 
what LLVM does by default. As long as no fast math flags are set, we will not 
perform optimizations that are not value safe.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc updated this revision to Diff 215666.
mibintc marked an inline comment as not done.
mibintc edited the summary of this revision.
mibintc added a comment.

I addressed some comments from @kpn: I corrected the documentation formatting 
and added some details, and used Diag instead of llvm_unreachable.

I decided to change -fp-model=except- to -fp-model=noexcept

When the user requests -fp-model=strict I explicitly set the FMA Contraction 
mode to off.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/fpconstrained.c
  clang/test/Driver/clang_f_opts.c

Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -217,6 +217,11 @@
 // RUN: %clang -### -S -fexec-charset=iso-8859-1 -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-INPUT-CHARSET %s
 // CHECK-INVALID-INPUT-CHARSET: error: invalid value 'iso-8859-1' in '-fexec-charset=iso-8859-1'
 
+// RUN: %clang -### -S -fp-model=fast -fp-model=except -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-FAST-EXCEPT %s
+// RUN: %clang -### -S -fp-model=fast -fp-model=except -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-FAST-EXCEPT %s
+// CHECK-INVALID-FAST-EXCEPT: error: invalid argument 'fp-model=fast' not allowed with 'fp-model=except'
+//
+
 // Test that we don't error on these.
 // RUN: %clang -### -S -Werror\
 // RUN: -falign-functions -falign-functions=2 -fno-align-functions\
Index: clang/test/CodeGen/fpconstrained.c
===
--- /dev/null
+++ clang/test/CodeGen/fpconstrained.c
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fp-model=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=STRICT
+// RUN: %clang_cc1 -fp-model=strict -fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=STRICTEXCEPT
+// RUN: %clang_cc1 -fp-model=strict -no-fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=STRICTNOEXCEPT
+// RUN: %clang_cc1 -fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=EXCEPT
+// RUN: %clang_cc1 -no-fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=NOEXCEPT
+// RUN: %clang_cc1 -fp-model=precise -emit-llvm -o - %s | FileCheck %s -check-prefix=PRECISE
+// RUN: %clang_cc1 -fp-model=fast -emit-llvm -o - %s | FileCheck %s -check-prefix=FAST
+// RUN: %clang_cc1 -fp-model=fast -fp-speculation=fast -emit-llvm -o - %s | FileCheck %s -check-prefix=NOEXCEPT
+// RUN: %clang_cc1 -fp-model=fast -fp-speculation=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=EXCEPT
+// RUN: %clang_cc1 -fp-model=fast -fp-speculation=safe -emit-llvm -o - %s | FileCheck %s -check-prefix=MAYTRAP
+float f0, f1, f2;
+
+void foo(void) {
+  // CHECK-LABEL: define {{.*}}void @foo()
+
+  // MAYTRAP: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.tonearest", metadata !"fpexcept.maytrap")
+  // EXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.tonearest", metadata !"fpexcept.strict")
+  // NOEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.tonearest", metadata !"fpexcept.ignore")
+  // STRICT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.dynamic", metadata !"fpexcept.strict")
+  // STRICTEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.dynamic", metadata !"fpexcept.strict")
+  // STRICTNOEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.dynamic", metadata !"fpexcept.ignore")
+  // PRECISE: fadd float
+  // FAST: fadd fast
+  f0 = f1 + f2;
+
+  // CHECK: ret
+}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3039,6 +3039,59 @@
   Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
   }
 
+  LangOptions::FPModelKind FPM = LangOptions::FPM_Off;
+  if (Arg *A = Args.getLastArg(OPT_fp_model_EQ)) {
+StringRef Val = A->getValue();
+if (Val == "precise")
+  FPM = LangOptions::FPM_Precise;
+else if (Val == "strict")
+  FPM = LangOptions::FPM_Strict;
+else if (Val == "fast")
+  FPM = LangOptions::FPM_Fast;
+else
+  llvm_unreachable("invalid -fp-model setting");
+  }
+  Opts.getFPMOptions().setFPModelSetting(FPM);
+
+  LangOptions::FPModelExceptKind FPME 

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 3 inline comments as not done.
mibintc added a comment.

I added an inline reply and unmarked some things that had been inadvertently 
marked done.




Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:126
+  case LangOptions::FPM_Precise:
+  case LangOptions::FPM_Fast:
+break;

mibintc wrote:
> kpn wrote:
> > Wait, so "fast" and "precise" are the same thing? That doesn't sound like 
> > where the documentation you put in the ticket says "the compiler preserves 
> > the source expression ordering and rounding properties of floating-point".
> > 
> > (Yes, I saw below where "fast" turns on the fast math flags but "precise" 
> > doesn't. That doesn't affect my point here.)
> "precise" doesn't necessitate the use of Constrained Intrinsics, And likewise 
> for "fast".   The words "compiler preserves the source expression ordering" 
> were copied from the msdn documentation for /fp:precise as you explained it 
> would be useful to have the msdn documentation for the option in case it goes 
> offline in, say, 30 years.  The ICL Intel compiler also provides equivalent 
> floating point options. The Intel documentation for precise is phrased 
> differently "Disables optimizations that are not value-safe on floating-point 
> data."  
> 
> fp-model=precise should enable contractions, if that's not true at default (I 
> mean, clang -c) then this patch is missing that.
> 
> fp-model=fast is the same as requesting ffast-math 
Well, we haven't heard from Andy yet, but he told me some time ago that 
/fp:precise corresponds more or less (there was wiggle room) to clang's default 
behavior.  It sounds like you think the description in the msdn of /fp:precise 
isn't describing clang's default behavior, @kpn can you say more about that, 
and do you think that ConstrainedIntrinsics should be created to provide the 
semantics of /fp:precise? 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 3 inline comments as done.
mibintc added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:126
+  case LangOptions::FPM_Precise:
+  case LangOptions::FPM_Fast:
+break;

kpn wrote:
> Wait, so "fast" and "precise" are the same thing? That doesn't sound like 
> where the documentation you put in the ticket says "the compiler preserves 
> the source expression ordering and rounding properties of floating-point".
> 
> (Yes, I saw below where "fast" turns on the fast math flags but "precise" 
> doesn't. That doesn't affect my point here.)
"precise" doesn't necessitate the use of Constrained Intrinsics, And likewise 
for "fast".   The words "compiler preserves the source expression ordering" 
were copied from the msdn documentation for /fp:precise as you explained it 
would be useful to have the msdn documentation for the option in case it goes 
offline in, say, 30 years.  The ICL Intel compiler also provides equivalent 
floating point options. The Intel documentation for precise is phrased 
differently "Disables optimizations that are not value-safe on floating-point 
data."  

fp-model=precise should enable contractions, if that's not true at default (I 
mean, clang -c) then this patch is missing that.

fp-model=fast is the same as requesting ffast-math 



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3052
+else
+  llvm_unreachable("invalid -fp-model setting");
+  }

kpn wrote:
> Shouldn't this be a call to Diags.Report() like in the code just above it and 
> below? Same question for _some_ other uses of llvm_unreachable().
I put it in as unreachable because the clang driver shouldn't build this 
combination, but that's a good point I can just switch it to match the other 
code in this function, thanks.



Comment at: clang/test/CodeGen/fpconstrained.c:22
+  // STRICTNOEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float 
%1, metadata !"round.dynamic", metadata !"fpexcept.ignore")
+  // PRECISE: fadd float
+  // FAST: fadd fast

kpn wrote:
> This is another case of "fast" and "precise" doing the same thing. If we're 
> using the regular fadd then it cannot be that "the compiler preserves the 
> source expression ordering and rounding properties of floating-point".
I need an fp wizard to address this point, @andrew.w.kaylor ??

The msdn documentation says that strict and precise both preserve ... 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Kevin P. Neal via Phabricator via cfe-commits]

kpn added inline comments.



Comment at: clang/docs/UsersManual.rst:1307
+
+   ``precise   `` Disables optimizations that are not value-safe on 
+   floating-point data, although FP contraction (FMA) is enabled.

Extra spaces?



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:126
+  case LangOptions::FPM_Precise:
+  case LangOptions::FPM_Fast:
+break;

Wait, so "fast" and "precise" are the same thing? That doesn't sound like where 
the documentation you put in the ticket says "the compiler preserves the source 
expression ordering and rounding properties of floating-point".

(Yes, I saw below where "fast" turns on the fast math flags but "precise" 
doesn't. That doesn't affect my point here.)



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3052
+else
+  llvm_unreachable("invalid -fp-model setting");
+  }

Shouldn't this be a call to Diags.Report() like in the code just above it and 
below? Same question for _some_ other uses of llvm_unreachable().



Comment at: clang/test/CodeGen/fpconstrained.c:22
+  // STRICTNOEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float 
%1, metadata !"round.dynamic", metadata !"fpexcept.ignore")
+  // PRECISE: fadd float
+  // FAST: fadd fast

This is another case of "fast" and "precise" doing the same thing. If we're 
using the regular fadd then it cannot be that "the compiler preserves the 
source expression ordering and rounding properties of floating-point".


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc updated this revision to Diff 215225.
mibintc added a comment.

I added documentation for the new floating point options into clang/docs


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/fpconstrained.c
  clang/test/Driver/clang_f_opts.c

Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -217,6 +217,11 @@
 // RUN: %clang -### -S -fexec-charset=iso-8859-1 -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-INPUT-CHARSET %s
 // CHECK-INVALID-INPUT-CHARSET: error: invalid value 'iso-8859-1' in '-fexec-charset=iso-8859-1'
 
+// RUN: %clang -### -S -fp-model=fast -fp-model=except -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-FAST-EXCEPT %s
+// RUN: %clang -### -S -fp-model=fast -fp-model=except -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-FAST-EXCEPT %s
+// CHECK-INVALID-FAST-EXCEPT: error: invalid argument 'fp-model=fast' not allowed with 'fp-model=except'
+//
+
 // Test that we don't error on these.
 // RUN: %clang -### -S -Werror\
 // RUN: -falign-functions -falign-functions=2 -fno-align-functions\
Index: clang/test/CodeGen/fpconstrained.c
===
--- /dev/null
+++ clang/test/CodeGen/fpconstrained.c
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fp-model=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=STRICT
+// RUN: %clang_cc1 -fp-model=strict -fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=STRICTEXCEPT
+// RUN: %clang_cc1 -fp-model=strict -no-fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=STRICTNOEXCEPT
+// RUN: %clang_cc1 -fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=EXCEPT
+// RUN: %clang_cc1 -no-fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=NOEXCEPT
+// RUN: %clang_cc1 -fp-model=precise -emit-llvm -o - %s | FileCheck %s -check-prefix=PRECISE
+// RUN: %clang_cc1 -fp-model=fast -emit-llvm -o - %s | FileCheck %s -check-prefix=FAST
+// RUN: %clang_cc1 -fp-model=fast -fp-speculation=fast -emit-llvm -o - %s | FileCheck %s -check-prefix=NOEXCEPT
+// RUN: %clang_cc1 -fp-model=fast -fp-speculation=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=EXCEPT
+// RUN: %clang_cc1 -fp-model=fast -fp-speculation=safe -emit-llvm -o - %s | FileCheck %s -check-prefix=MAYTRAP
+float f0, f1, f2;
+
+void foo(void) {
+  // CHECK-LABEL: define {{.*}}void @foo()
+
+  // MAYTRAP: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.tonearest", metadata !"fpexcept.maytrap")
+  // EXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.tonearest", metadata !"fpexcept.strict")
+  // NOEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.tonearest", metadata !"fpexcept.ignore")
+  // STRICT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.dynamic", metadata !"fpexcept.strict")
+  // STRICTEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.dynamic", metadata !"fpexcept.strict")
+  // STRICTNOEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.dynamic", metadata !"fpexcept.ignore")
+  // PRECISE: fadd float
+  // FAST: fadd fast
+  f0 = f1 + f2;
+
+  // CHECK: ret
+}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3039,6 +3039,59 @@
   Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
   }
 
+  LangOptions::FPModelKind FPM = LangOptions::FPM_Off;
+  if (Arg *A = Args.getLastArg(OPT_fp_model_EQ)) {
+StringRef Val = A->getValue();
+if (Val == "precise")
+  FPM = LangOptions::FPM_Precise;
+else if (Val == "strict")
+  FPM = LangOptions::FPM_Strict;
+else if (Val == "fast")
+  FPM = LangOptions::FPM_Fast;
+else
+  llvm_unreachable("invalid -fp-model setting");
+  }
+  Opts.getFPMOptions().setFPModelSetting(FPM);
+
+  LangOptions::FPModelExceptKind FPME = LangOptions::FPME_Off;
+  if (const Arg *A =
+  Args.getLastArg(OPT_fp_model_except, OPT_no_fp_model_except))
+switch (A->getOption().getID()) {
+case OPT_fp_model_except:
+  FPME = LangOptions::FPME_Except;
+  break;
+case OPT_no_fp_model_except:
+  FPME = 

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

Here's a summary of how the rm and eb options are set

if user requests fp-model=strict, then the ConstrainedIntrinsic will be built 
with ( rmDynamic, ebStrict )

if user requests fp-model=except or fp-model=noexcept then the 
ConstrainedIntrinsic will be built with eb set to Strict or Ignore, 
respectively.  In all cases, if the user options don't otherwise request a 
value for the rounding mode, but the user options has requested a value for 
excepton behavior, then the ConstrainedIntrinsic will use rmToNearest.

The fp-speculation option controls only the eb setting.  There are 3 possible 
values: fast, strict, safe and the ConstrainedIntrinsic will be built with eb 
set to ebIgnore, ebStrict, ebMayTrap respectively.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added a comment.

In D62731#1623114 , @mibintc wrote:

> @andrew.w.kaylor Thanks Andy.  Reminder -- in a private document you 
> indicated to me that -fp-speculation=safe corresponds to the maytrap setting 
> for the exception argument.  This patch is implemented with those semantics.


Right. This is a clear indication of why I shouldn't try to squeeze in a review 
just before heading out the door at the end of the day. The "separate option" 
in icc that I was referring to is -fp-speculation. Somehow I overlooked the 
fact that you were adding that option here. I saw your note and read the recent 
comments without looking over even the patch description. Sorry about that.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.



In D62731#1620174 , @andrew.w.kaylor 
wrote:

> I'm not entirely caught up on this review. I've only read the most recent 
> comments, but I think I've got enough context to comment on the metadata 
> arguments.
>
> Based only on the fp-model command line options, the front end should only 
> ever use "round.dynamic" (for strict mode) or "round.tonearest" (where full 
> fenv access isn't permitted but we want to enable strict




In D62731#1620174 , @andrew.w.kaylor 
wrote:

> I'm not entirely caught up on this review. I've only read the most recent 
> comments, but I think I've got enough context to comment on the metadata 
> arguments.
>
> Based only on the fp-model command line options, the front end should only 
> ever use "round.dynamic" (for strict mode) or "round.tonearest" (where full 
> fenv access isn't permitted but we want to enable strict exception 
> semantics). There are some pragmas, which I believe are in some draft of a 
> standards document but not yet approved, which can declare any of the other 
> rounding modes, but I don't know that we have plans to implement those yet. 
> I'm also hoping that at some point we'll have a pass that finds calls to 
> fesetround() and changes the metadata argument when it can prove what the 
> rounding mode will be.
>
> For the fp exception argument, the only values that can be implied by the 
> -fp-model option are "fpexcept.strict" and "fpexcept.ignore". In icc, we have 
> a separate option that can prevent speculation (equivalent to 
> "fpexcept.maytrap"). I think gcc's, -ftrapping-math has a similar function 
> (though the default may be reversed). I don't think we've talked about how 
> (or if) clang should ever get into the "fpexcept.maytrap" state.
>
> So for now, I think both arguments only need to support one of two states, 
> depending on the -fp-model arguments.




In D62731#1620174 , @andrew.w.kaylor 
wrote:

> I'm not entirely caught up on this review. I've only read the most recent 
> comments, but I think I've got enough context to comment on the metadata 
> arguments.
>
> Based only on the fp-model command line options, the front end should only 
> ever use "round.dynamic" (for strict mode) or "round.tonearest" (where full 
> fenv access isn't permitted but we want to enable strict exception 
> semantics). There are some pragmas, which I believe are in some draft of a 
> standards document but not yet approved, which can declare any of the other 
> rounding modes, but I don't know that we have plans to implement those yet. 
> I'm also hoping that at some point we'll have a pass that finds calls to 
> fesetround() and changes the metadata argument when it can prove what the 
> rounding mode will be.
>
> For the fp exception argument, the only values that can be implied by the 
> -fp-model option are "fpexcept.strict" and "fpexcept.ignore". In icc, we have 
> a separate option that can prevent speculation (equivalent to 
> "fpexcept.maytrap"). I think gcc's, -ftrapping-math has a similar function 
> (though the default may be reversed). I don't think we've talked about how 
> (or if) clang should ever get into the "fpexcept.maytrap" state.
>
> So for now, I think both arguments only need to support one of two states, 
> depending on the -fp-model arguments.


@andrew.w.kaylor Thanks Andy.  Reminder -- in a private document you indicated 
to me that -fp-speculation=safe corresponds to the maytrap setting for the 
exception argument.  This patch is implemented with those semantics.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added a comment.

I'm not entirely caught up on this review. I've only read the most recent 
comments, but I think I've got enough context to comment on the metadata 
arguments.

Based only on the fp-model command line options, the front end should only ever 
use "round.dynamic" (for strict mode) or "round.tonearest" (where full fenv 
access isn't permitted but we want to enable strict exception semantics). There 
are some pragmas, which I believe are in some draft of a standards document but 
not yet approved, which can declare any of the other rounding modes, but I 
don't know that we have plans to implement those yet. I'm also hoping that at 
some point we'll have a pass that finds calls to fesetround() and changes the 
metadata argument when it can prove what the rounding mode will be.

For the fp exception argument, the only values that can be implied by the 
-fp-model option are "fpexcept.strict" and "fpexcept.ignore". In icc, we have a 
separate option that can prevent speculation (equivalent to 
"fpexcept.maytrap"). I think gcc's, -ftrapping-math has a similar function 
(though the default may be reversed). I don't think we've talked about how (or 
if) clang should ever get into the "fpexcept.maytrap" state.

So for now, I think both arguments only need to support one of two states, 
depending on the -fp-model arguments.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

In D62731#1619908 , @lebedev.ri wrote:

> In D62731#1619892 , @mibintc wrote:
>
> > Compared to the 2nd revision, this patch moves all the changes into clang, 
> > removing the FPState file.
> >
> > In the summary, I've copied information from Microsoft about the fp-model 
> > options into the differential Summary, as requested by Kevin, to create a 
> > legacy for future maintainers in case that information disappears from msdn.
>
>
> The the documentation should be documentation, it should reside somewhere in 
> `clang/docs/`.


Got it thanks


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

In D62731#1603030 , @kpn wrote:

> I actually don't have much of an opinion on what the command line argument 
> form should be. It may be helpful for it to be the same as one of the 
> commonly deployed compilers. The worst I think would be pretty close but with 
> subtle differences. So if it can be made to work like Intel's compiler I'm 
> fine with that. But I'm hoping that more people in the community chime in 
> since having a consensus would be best. Personally, I'm not yet giving any 
> final sign-offs to tickets since I don't think I've been here long enough.
>
> As far as the rounding metadata argument, the language reference says this:
>
> > For values other than “round.dynamic” optimization passes may assume that 
> > the actual runtime rounding mode (as defined in a target-specific manner) 
> > matches the specified rounding mode, but this is not guaranteed. Using a 
> > specific non-dynamic rounding mode which does not match the actual rounding 
> > mode at runtime results in undefined behavior.
>
> Be aware that currently neither of the metadata arguments does anything. They 
> get dropped when llvm reaches the SelectionDAG. And none of the optimization 
> passes that run before that know anything about constrained intrinsics at 
> all. This means they treat code that has them conservatively. Preserving and 
> using that metadata in the optimization passes and getting it down and used 
> by the MI layer is planned but hasn't happened yet. So the full set of 
> arguments may not make sense yet, but an on/off switch for strict mode 
> hopefully does.


@andrew.w.kaylor Can you please check over Kevin's comments about metadata?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

In D62731#1601408 , @kpn wrote:

> In D62731#1601310 , @mibintc wrote:
>
> > I think it would be convenient to have an "unset" setting for the different 
> > constrained modes, otherwise you need a boolean that says "no value was 
> > provided for this option".  But i'm a frontend person so I may need to have 
> > my attitude adjusted.
>
>
> What "different constrained modes"? The IRBuilder is either in constrained 
> mode or it isn't. In constrained mode the exception behavior and rounding 
> mode both have defaults, and those defaults can be changed individually 
> without affecting the other setting. The current defaults can also be 
> retrieved if you need something for a function call where you don't want to 
> change it but need an argument anyway. When do you need this "no value 
> provided" setting?
>
> Oh, I'd check the tools/clang/CODE_OWNERS.txt file and add additional 
> appropriate reviewers. Perhaps John McCall and Richard Smith? I don't know 
> who has opinions on how command line options should be handled.
>
> Do we want the Unix driver to be compatible with gcc? Maybe, maybe not. 
> Opinions, anyone?


Yes we need opinions on this.

> The documentation request from lebedev.ri isn't in this ticket yet.

I'm not yet sure what I need for this, the requested documentation is still 
missing.

> Also, for future historical research purposes I'd cut and paste the relevant 
> portions of outside web pages (like intel.com's) and post them somewhere 
> llvm-ish where they are findable. This ticket, for example, is a good place. 
> Web sites gets reorganized or vanish in full or in part. It's helpful to have 
> some insulation from that over time. I've had to fix compiler bugs that 
> actually were 25 years old before. Yes, 25 years old. Being able to do the 
> research is very helpful.
> 
> Oh, it may be useful to know that constrained floating point and the 
> FastMathFlags are not mutually exclusive. I don't know if that matters here 
> or not, but you did mention FastMathFlags.

Yes they aren't mutually exclusive.  One of the fp-model options implies 
enabling fast math.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D62731#1619892 , @mibintc wrote:

> Compared to the 2nd revision, this patch moves all the changes into clang, 
> removing the FPState file.
>
> In the summary, I've copied information from Microsoft about the fp-model 
> options into the differential Summary, as requested by Kevin, to create a 
> legacy for future maintainers in case that information disappears from msdn.


The the documentation should be documentation, it should reside somewhere in 
`clang/docs/`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc updated this revision to Diff 214003.
mibintc edited the summary of this revision.
mibintc added a comment.

Compared to the 2nd revision, this patch moves all the changes into clang, 
removing the FPState file.

In the summary, I've copied information from Microsoft about the fp-model 
options into the differential Summary, as requested by Kevin, to create a 
legacy for future maintainers in case that information disappears from msdn.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/fpconstrained.c
  clang/test/Driver/clang_f_opts.c

Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -217,6 +217,11 @@
 // RUN: %clang -### -S -fexec-charset=iso-8859-1 -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-INPUT-CHARSET %s
 // CHECK-INVALID-INPUT-CHARSET: error: invalid value 'iso-8859-1' in '-fexec-charset=iso-8859-1'
 
+// RUN: %clang -### -S -fp-model=fast -fp-model=except -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-FAST-EXCEPT %s
+// RUN: %clang -### -S -fp-model=fast -fp-model=except -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-FAST-EXCEPT %s
+// CHECK-INVALID-FAST-EXCEPT: error: invalid argument 'fp-model=fast' not allowed with 'fp-model=except'
+//
+
 // Test that we don't error on these.
 // RUN: %clang -### -S -Werror\
 // RUN: -falign-functions -falign-functions=2 -fno-align-functions\
Index: clang/test/CodeGen/fpconstrained.c
===
--- /dev/null
+++ clang/test/CodeGen/fpconstrained.c
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fp-model=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=STRICT
+// RUN: %clang_cc1 -fp-model=strict -fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=STRICTEXCEPT
+// RUN: %clang_cc1 -fp-model=strict -no-fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=STRICTNOEXCEPT
+// RUN: %clang_cc1 -fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=EXCEPT
+// RUN: %clang_cc1 -no-fp-model-except -emit-llvm -o - %s | FileCheck %s -check-prefix=NOEXCEPT
+// RUN: %clang_cc1 -fp-model=precise -emit-llvm -o - %s | FileCheck %s -check-prefix=PRECISE
+// RUN: %clang_cc1 -fp-model=fast -emit-llvm -o - %s | FileCheck %s -check-prefix=FAST
+// RUN: %clang_cc1 -fp-model=fast -fp-speculation=fast -emit-llvm -o - %s | FileCheck %s -check-prefix=NOEXCEPT
+// RUN: %clang_cc1 -fp-model=fast -fp-speculation=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=EXCEPT
+// RUN: %clang_cc1 -fp-model=fast -fp-speculation=safe -emit-llvm -o - %s | FileCheck %s -check-prefix=MAYTRAP
+float f0, f1, f2;
+
+void foo(void) {
+  // CHECK-LABEL: define {{.*}}void @foo()
+
+  // MAYTRAP: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.tonearest", metadata !"fpexcept.maytrap")
+  // EXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.tonearest", metadata !"fpexcept.strict")
+  // NOEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.tonearest", metadata !"fpexcept.ignore")
+  // STRICT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.dynamic", metadata !"fpexcept.strict")
+  // STRICTEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.dynamic", metadata !"fpexcept.strict")
+  // STRICTNOEXCEPT: llvm.experimental.constrained.fadd.f32(float %0, float %1, metadata !"round.dynamic", metadata !"fpexcept.ignore")
+  // PRECISE: fadd float
+  // FAST: fadd fast
+  f0 = f1 + f2;
+
+  // CHECK: ret
+}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3036,6 +3036,59 @@
   Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
   }
 
+  LangOptions::FPModelKind FPM = LangOptions::FPM_Off;
+  if (Arg *A = Args.getLastArg(OPT_fp_model_EQ)) {
+StringRef Val = A->getValue();
+if (Val == "precise")
+  FPM = LangOptions::FPM_Precise;
+else if (Val == "strict")
+  FPM = LangOptions::FPM_Strict;
+else if (Val == "fast")
+  FPM = LangOptions::FPM_Fast;
+else
+  llvm_unreachable("invalid -fp-model setting");
+  }
+  Opts.getFPMOptions().setFPModelSetting(FPM);
+
+  LangOptions::FPModelExceptKind FPME = LangOptions::FPME_Off;
+  if (const Arg 

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Kevin P. Neal via Phabricator via cfe-commits]

kpn added a comment.

I actually don't have much of an opinion on what the command line argument form 
should be. It may be helpful for it to be the same as one of the commonly 
deployed compilers. The worst I think would be pretty close but with subtle 
differences. So if it can be made to work like Intel's compiler I'm fine with 
that. But I'm hoping that more people in the community chime in since having a 
consensus would be best. Personally, I'm not yet giving any final sign-offs to 
tickets since I don't think I've been here long enough.

As far as the rounding metadata argument, the language reference says this:

> For values other than “round.dynamic” optimization passes may assume that the 
> actual runtime rounding mode (as defined in a target-specific manner) matches 
> the specified rounding mode, but this is not guaranteed. Using a specific 
> non-dynamic rounding mode which does not match the actual rounding mode at 
> runtime results in undefined behavior.

Be aware that currently neither of the metadata arguments does anything. They 
get dropped when llvm reaches the SelectionDAG. And none of the optimization 
passes that run before that know anything about constrained intrinsics at all. 
This means they treat code that has them conservatively. Preserving and using 
that metadata in the optimization passes and getting it down and used by the MI 
layer is planned but hasn't happened yet. So the full set of arguments may not 
make sense yet, but an on/off switch for strict mode hopefully does.




Comment at: llvm/lib/IR/FPState.cpp:78
+  }
+
+  Builder.setIsFPConstrained(IsConstrainedExcept || IsConstrainedRounding);

mibintc wrote:
> kpn wrote:
> > The IRBuilder already has defaults for exception behavior and rounding. Is 
> > it a good idea to duplicate that knowledge here? Worse, what's here is 
> > different from what's in the IRBuilder. Why not ask the IRBuilder what its 
> > current setting is and use that?
> > 
> > Would it make sense to have setters/getters, and then have a separate 
> > updateBuilder() method? We still don't have a good way to get the #pragma 
> > down to the lower levels of clang. The current, unfinished, attempt doesn't 
> > work for C++ templates and I'm told other cases as well. An updateBuilder() 
> > method could be helpful when moving from one scope to another. But keep in 
> > mind that if any constrained fp math is used in a function then the entire 
> > function has to be constrained.
> > 
> > Given that last bit, it may make more sense to have the support somewhere 
> > higher level than as a wrapper around the IRBuilder. Maybe in 
> > CodeGenFunction or CodeGenModule? I've not spent much time in clang so I'm 
> > not sure if that makes sense or not.
> Yes I absolutely don't want to duplicate, and I will submit another version 
> of this patch and i'll be removing fpstate*.  I do want to be able to make 
> the fp-model options match the same behavior as icc is using.  One reason i 
> wanted to keep track of the state within a separate object is because i was 
> uncertain if stuff would be going on in the IR builder which would be 
> changing the settings, for whatever reason, and i'd want to put them back 
> into settings specified by the command line options before creating the 
> constrained intrinsics in clang/codegen. 
> 
> let me work on this patch more, i just did a quick update to the latest 
> before sending this up.
> 
> As far as pragmas versus templates, this is a concern.  Is there something I 
> can read to learn more about the issue?  Pragma's are used in OpenMP so there 
> must be a way to have pragma's interact politely with templates?  Knowing 
> very little, I thought that the pragma would be held as a _Pragma, sort of 
> like a function call, within the intermediate representation e.g. as opposed 
> to a token stream from the preprocessor and I didn't think there would be a 
> problem with templates per se. I'll check with other folks here at Intel. 
> There was  a concern about inlining constrained intrinsics into a function 
> because of your rule about whole function body but nobody mentioned a problem 
> with templates.
See "https://reviews.llvm.org/D52839;, "Inform AST's UnaryOperator of 
FENV_ACCESS". It was there that Richard Smith brought up the issue of 
templates. I've been prioritizing work on the llvm end and haven't had a chance 
to get to understand how the relevant parts on the clang side work myself. My 
hope is that maybe command line arguments can go in to enable strict FP on a 
compilation-wide basis, with support for the #pragma coming later. But I don't 
know if it will work out that way.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 3 inline comments as done.
mibintc added a comment.

Thanks for your review >>! In D62731#1601408 
, @kpn wrote:

> In D62731#1601310 , @mibintc wrote:
> 
>> I think it would be convenient to have an "unset" setting for the different 
>> constrained modes, otherwise you need a boolean that says "no value was 
>> provided for this option".  But i'm a frontend person so I may need to have 
>> my attitude adjusted.
> 
> 
> What "different constrained modes"? The IRBuilder is either in constrained 
> mode or it isn't. In constrained mode the exception behavior and rounding 
> mode both have defaults, and those defaults can be changed individually 
> without affecting the other setting. The current defaults can also be 
> retrieved if you need something for a function call where you don't want to 
> change it but need an argument anyway. When do you need this "no value 
> provided" setting?

I'm going to rewrite this

> Oh, I'd check the tools/clang/CODE_OWNERS.txt file and add additional 
> appropriate reviewers. Perhaps John McCall and Richard Smith? I don't know 
> who has opinions on how command line options should be handled.

I'd like to fix it more before I add more reviewers

> Do we want the Unix driver to be compatible with gcc? Maybe, maybe not. 
> Opinions, anyone?

Oh, I think you mean something more like the gnuish -fno-except or maybe 
-fp-model=no-except? instead of -fp-model=except- ok we can get that sorted.

> The documentation request from lebedev.ri isn't in this ticket yet.
> 
> Also, for future historical research purposes I'd cut and paste the relevant 
> portions of outside web pages (like intel.com's) and post them somewhere 
> llvm-ish where they are findable. This ticket, for example, is a good place. 
> Web sites gets reorganized or vanish in full or in part. It's helpful to have 
> some insulation from that over time. I've had to fix compiler bugs that 
> actually were 25 years old before. Yes, 25 years old. Being able to do the 
> research is very helpful.

That's a good idea thanks

> Oh, it may be useful to know that constrained floating point and the 
> FastMathFlags are not mutually exclusive. I don't know if that matters here 
> or not, but you did mention FastMathFlags.

Yes i'm not sure how the fast math command line optoins should interact with 
the fp-model options, i'll have to dig into that.




Comment at: llvm/lib/IR/FPState.cpp:1
+#include "llvm/IR/FPState.h"
+#include "llvm/IR/IRBuilder.h"

arsenm wrote:
> Missing license header
Thanks @arsenm in the end i believe i won't be adding this file



Comment at: llvm/lib/IR/FPState.cpp:73
+  if (IsConstrainedExcept && !IsConstrainedRounding) {
+// If the rounding mode isn't set explicitly above, then use ebToNearest
+// as the value when the constrained intrinsic is created

arsenm wrote:
> eb?
I mean, if the user requested constrained exception via the option 
-fp-model=except but no rounding mode has been requested (again via command 
line options) then the rounding mode should be set to "round to nearest".  I'm 
following a description of how the icc compiler works. I'm afraid that your 
concise comment "eb?" doesn't convey enough information for me to understand 
what you mean.  With these further remarks is it clear now? 



Comment at: llvm/lib/IR/FPState.cpp:78
+  }
+
+  Builder.setIsFPConstrained(IsConstrainedExcept || IsConstrainedRounding);

kpn wrote:
> The IRBuilder already has defaults for exception behavior and rounding. Is it 
> a good idea to duplicate that knowledge here? Worse, what's here is different 
> from what's in the IRBuilder. Why not ask the IRBuilder what its current 
> setting is and use that?
> 
> Would it make sense to have setters/getters, and then have a separate 
> updateBuilder() method? We still don't have a good way to get the #pragma 
> down to the lower levels of clang. The current, unfinished, attempt doesn't 
> work for C++ templates and I'm told other cases as well. An updateBuilder() 
> method could be helpful when moving from one scope to another. But keep in 
> mind that if any constrained fp math is used in a function then the entire 
> function has to be constrained.
> 
> Given that last bit, it may make more sense to have the support somewhere 
> higher level than as a wrapper around the IRBuilder. Maybe in CodeGenFunction 
> or CodeGenModule? I've not spent much time in clang so I'm not sure if that 
> makes sense or not.
Yes I absolutely don't want to duplicate, and I will submit another version of 
this patch and i'll be removing fpstate*.  I do want to be able to make the 
fp-model options match the same behavior as icc is using.  One reason i wanted 
to keep track of the state within a separate object is because i was uncertain 
if stuff would be going on in the IR 

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: llvm/include/llvm/IR/FPState.h:1
+#ifndef LLVM_FPSTATE_H
+#define LLVM_FPSTATE_H

Missing license header and c++ mode comment



Comment at: llvm/lib/IR/FPState.cpp:1
+#include "llvm/IR/FPState.h"
+#include "llvm/IR/IRBuilder.h"

Missing license header



Comment at: llvm/lib/IR/FPState.cpp:73
+  if (IsConstrainedExcept && !IsConstrainedRounding) {
+// If the rounding mode isn't set explicitly above, then use ebToNearest
+// as the value when the constrained intrinsic is created

eb?



Comment at: llvm/unittests/IR/IRBuilderTest.cpp:205
+  V = Builder.CreateFAdd(V, V);
+  ASSERT_TRUE(!isa(V));
+

ASSERT_FALSE with no !



Comment at: llvm/unittests/IR/IRBuilderTest.cpp:217-218
+  ASSERT_TRUE(isa(V));
+  ASSERT_TRUE(CII->getExceptionBehavior() == ConstrainedFPIntrinsic::ebStrict);
+  ASSERT_TRUE(CII->getRoundingMode() == ConstrainedFPIntrinsic::rmDynamic);
+

Most of these should not be using ASSERT_, and instead EXPECT_EQ



Comment at: llvm/unittests/IR/IRBuilderTest.cpp:258
+  CII = cast(V);
+  ASSERT_TRUE(isa(V));
+  ASSERT_TRUE(CII->getExceptionBehavior() == 
ConstrainedFPIntrinsic::ebMayTrap);

This already would have crashed from the cast<> before



Comment at: llvm/unittests/IR/IRBuilderTest.cpp:259-260
+  ASSERT_TRUE(isa(V));
+  ASSERT_TRUE(CII->getExceptionBehavior() == 
ConstrainedFPIntrinsic::ebMayTrap);
+  ASSERT_TRUE(CII->getRoundingMode() == ConstrainedFPIntrinsic::rmToNearest);
 

EXPECT_EQ


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: [PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Blower, Melanie via cfe-commits]

Thanks for your review. I put some quick replies below.  I'll work on an update 
to this. 

> -Original Message-
> From: Kevin P. Neal via Phabricator [mailto:revi...@reviews.llvm.org]
> Sent: Thursday, July 25, 2019 1:09 PM
> To: Blower, Melanie ; chandl...@gmail.com
> Cc: mgo...@gentoo.org; hiradi...@msn.com; wuz...@cn.ibm.com; Wang,
> Pengfei ; lebedev...@gmail.com;
> cameron.mcina...@nyu.edu; llvm-comm...@lists.llvm.org; Rice, Michael P
> ; Kaylor, Andrew ;
> kevin.n...@sas.com; cfe-commits@lists.llvm.org; paul.robin...@sony.com;
> peter.wal...@arm.com
> Subject: [PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-
> speculation= : specify floating point behavior
> 
> kpn added a comment.
> 
> In D62731#1601310 <https://reviews.llvm.org/D62731#1601310>, @mibintc
> wrote:
> 
> > I think it would be convenient to have an "unset" setting for the different
> constrained modes, otherwise you need a boolean that says "no value was
> provided for this option".  But i'm a frontend person so I may need to have my
> attitude adjusted.
> 
> 
> What "different constrained modes"? The IRBuilder is either in constrained
> mode or it isn't. In constrained mode the exception behavior and rounding
> mode both have defaults, and those defaults can be changed individually
> without affecting the other setting. The current defaults can also be 
> retrieved if
> you need something for a function call where you don't want to change it but
> need an argument anyway. When do you need this "no value provided" setting?
[Blower, Melanie] I was thinking it would be convenient to have null values for 
RoundingMode and ExceptionBehavior but I'll work on the patch more in this area
> 
> Oh, I'd check the tools/clang/CODE_OWNERS.txt file and add additional
> appropriate reviewers. Perhaps John McCall and Richard Smith? I don't know
> who has opinions on how command line options should be handled.
[Blower, Melanie] Good idea but I'm not quite ready yet
> 
> Do we want the Unix driver to be compatible with gcc? Maybe, maybe not.
> Opinions, anyone?
[Blower, Melanie] For Intel's icc compiler, we support options describing 
fp-model in both Linux and Windows. Our customers find it useful. I want to add 
it both places. Gcc doesn't support these options.
> 
> The documentation request from lebedev.ri isn't in this ticket yet.
> 
> Also, for future historical research purposes I'd cut and paste the relevant
> portions of outside web pages (like intel.com's) and post them somewhere llvm-
> ish where they are findable. This ticket, for example, is a good place. Web 
> sites
> gets reorganized or vanish in full or in part. It's helpful to have some 
> insulation
> from that over time. I've had to fix compiler bugs that actually were 25 
> years old
> before. Yes, 25 years old. Being able to do the research is very helpful.
[Blower, Melanie] good idea, I will address that, adding it to this patch 
description? Or inline as comments in the source file itself?  (I may be able 
to compete with you on oldest bug fixed...)
> 
> Oh, it may be useful to know that constrained floating point and the
> FastMathFlags are not mutually exclusive. I don't know if that matters here or
> not, but you did mention FastMathFlags.
[Blower, Melanie] yes I worry about that
> 
> 
> 
> 
> Comment at: llvm/lib/IR/FPState.cpp:78
> +  }
> +
> +  Builder.setIsFPConstrained(IsConstrainedExcept || IsConstrainedRounding);
> 
> The IRBuilder already has defaults for exception behavior and rounding. Is it 
> a
> good idea to duplicate that knowledge here? Worse, what's here is different
> from what's in the IRBuilder. Why not ask the IRBuilder what its current 
> setting
> is and use that?
[Blower, Melanie] I was/am/ trying to translate the fp-model command line 
options semantics. I will look at this again.
> 
> Would it make sense to have setters/getters, and then have a separate
> updateBuilder() method? We still don't have a good way to get the #pragma
> down to the lower levels of clang. The current, unfinished, attempt doesn't 
> work
> for C++ templates and I'm told other cases as well. An updateBuilder() method
> could be helpful when moving from one scope to another. But keep in mind that
> if any constrained fp math is used in a function then the entire function has 
> to be
> constrained.
[Blower, Melanie] hmm
> 
> Given that last bit, it may make more sense to have the support somewhere
> higher level than as a wrapper around the IRBuilder. Maybe in
> CodeGenFunction or CodeGenModule? I've not spent much time in clang so I'm
> not sure if that makes sense or not.
> 
> 
> Repository:
>   rL LLVM
> 
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D62731/new/
> 
> https://reviews.llvm.org/D62731
> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Kevin P. Neal via Phabricator via cfe-commits]

kpn added a comment.

In D62731#1601310 , @mibintc wrote:

> I think it would be convenient to have an "unset" setting for the different 
> constrained modes, otherwise you need a boolean that says "no value was 
> provided for this option".  But i'm a frontend person so I may need to have 
> my attitude adjusted.


What "different constrained modes"? The IRBuilder is either in constrained mode 
or it isn't. In constrained mode the exception behavior and rounding mode both 
have defaults, and those defaults can be changed individually without affecting 
the other setting. The current defaults can also be retrieved if you need 
something for a function call where you don't want to change it but need an 
argument anyway. When do you need this "no value provided" setting?

Oh, I'd check the tools/clang/CODE_OWNERS.txt file and add additional 
appropriate reviewers. Perhaps John McCall and Richard Smith? I don't know who 
has opinions on how command line options should be handled.

Do we want the Unix driver to be compatible with gcc? Maybe, maybe not. 
Opinions, anyone?

The documentation request from lebedev.ri isn't in this ticket yet.

Also, for future historical research purposes I'd cut and paste the relevant 
portions of outside web pages (like intel.com's) and post them somewhere 
llvm-ish where they are findable. This ticket, for example, is a good place. 
Web sites gets reorganized or vanish in full or in part. It's helpful to have 
some insulation from that over time. I've had to fix compiler bugs that 
actually were 25 years old before. Yes, 25 years old. Being able to do the 
research is very helpful.

Oh, it may be useful to know that constrained floating point and the 
FastMathFlags are not mutually exclusive. I don't know if that matters here or 
not, but you did mention FastMathFlags.




Comment at: llvm/lib/IR/FPState.cpp:78
+  }
+
+  Builder.setIsFPConstrained(IsConstrainedExcept || IsConstrainedRounding);

The IRBuilder already has defaults for exception behavior and rounding. Is it a 
good idea to duplicate that knowledge here? Worse, what's here is different 
from what's in the IRBuilder. Why not ask the IRBuilder what its current 
setting is and use that?

Would it make sense to have setters/getters, and then have a separate 
updateBuilder() method? We still don't have a good way to get the #pragma down 
to the lower levels of clang. The current, unfinished, attempt doesn't work for 
C++ templates and I'm told other cases as well. An updateBuilder() method could 
be helpful when moving from one scope to another. But keep in mind that if any 
constrained fp math is used in a function then the entire function has to be 
constrained.

Given that last bit, it may make more sense to have the support somewhere 
higher level than as a wrapper around the IRBuilder. Maybe in CodeGenFunction 
or CodeGenModule? I've not spent much time in clang so I'm not sure if that 
makes sense or not.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc updated this revision to Diff 211770.
mibintc added a comment.
Herald added subscribers: hiraditya, mgorny.

The IRBuilder now has  been taught about constrained fadd and friends.  I 
simply updated my patches to work with the committed revision.  Note that this 
diff now contains what was formerly being reviewed separately in clang+llvm.  
Also let's discuss the new llvm file fpState.h. In this revision I didn't spend 
a lot of time pondering the best way to support this in the current world, i 
just made a simple update so it would build.

I could put the entire patch under clang and not create new files in llvm.  I 
think it would be an advantage to have the interpretation of the floating point 
command line switches occur in llvm itself. For one reason, because Intel plans 
to contribute the same options in the Fortran compiler as well as the clang 
compiler. This puts the switch logic into a single place.  Other languages may 
want to add support for the options too. Note, we also plan to implement inline 
pragma's for clang which allow the fp modes to be set for the duration of a 
code block.

I think it would be convenient to have an "unset" setting for the different 
constrained modes, otherwise you need a boolean that says "no value was 
provided for this option".  But i'm a frontend person so I may need to have my 
attitude adjusted.

When I was coding this before, I needed to pull out FPState.h from within 
IRBuilder because otherwise I needed to sprinkle additional include directives 
in many clang files, if it's pulled out like this to be more or less standalone 
then that problem resolved.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/clang_f_opts.c
  llvm/include/llvm/IR/FPState.h
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/FPState.cpp
  llvm/unittests/IR/IRBuilderTest.cpp

Index: llvm/unittests/IR/IRBuilderTest.cpp
===
--- llvm/unittests/IR/IRBuilderTest.cpp
+++ llvm/unittests/IR/IRBuilderTest.cpp
@@ -10,6 +10,7 @@
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/FPState.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
@@ -195,6 +196,68 @@
   EXPECT_EQ(CII->getIntrinsicID(), Intrinsic::experimental_constrained_fadd);
   ASSERT_TRUE(CII->getExceptionBehavior() == ConstrainedFPIntrinsic::ebMayTrap);
   ASSERT_TRUE(CII->getRoundingMode() == ConstrainedFPIntrinsic::rmDownward);
+  //
+  // Use FPState to update the builder settings
+  FPState fpState(Builder);
+
+  fpState.updateBuilder(FPState::FPM_Off, FPState::FPME_Off, FPState::FPS_Off);
+  V = Builder.CreateFAdd(V, V);
+  ASSERT_TRUE(!isa(V));
+
+  fpState.updateBuilder(FPState::FPM_Precise, FPState::FPME_Off,
+FPState::FPS_Off);
+  V = Builder.CreateFAdd(V, V);
+  ASSERT_TRUE(!isa(V));
+
+  fpState.updateBuilder(FPState::FPM_Strict, FPState::FPME_Off,
+FPState::FPS_Off);
+  V = Builder.CreateFAdd(V, V);
+  CII = cast(V);
+  ASSERT_TRUE(isa(V));
+  ASSERT_TRUE(CII->getExceptionBehavior() == ConstrainedFPIntrinsic::ebStrict);
+  ASSERT_TRUE(CII->getRoundingMode() == ConstrainedFPIntrinsic::rmDynamic);
+
+  fpState.updateBuilder(FPState::FPM_Off, FPState::FPME_Except,
+FPState::FPS_Off);
+  V = Builder.CreateFAdd(V, V);
+  CII = cast(V);
+  ASSERT_TRUE(isa(V));
+  ASSERT_TRUE(CII->getExceptionBehavior() == ConstrainedFPIntrinsic::ebStrict);
+  ASSERT_TRUE(CII->getRoundingMode() == ConstrainedFPIntrinsic::rmToNearest);
+
+  fpState.updateBuilder(FPState::FPM_Off, FPState::FPME_NoExcept,
+FPState::FPS_Off);
+  V = Builder.CreateFAdd(V, V);
+  CII = cast(V);
+  ASSERT_TRUE(isa(V));
+  ASSERT_TRUE(CII->getExceptionBehavior() == ConstrainedFPIntrinsic::ebIgnore);
+  ASSERT_TRUE(CII->getRoundingMode() == ConstrainedFPIntrinsic::rmToNearest);
+
+  fpState.updateBuilder(FPState::FPM_Fast, FPState::FPME_Off, FPState::FPS_Off);
+  V = Builder.CreateFAdd(V, V);
+  ASSERT_TRUE(!isa(V));
+
+  fpState.updateBuilder(FPState::FPM_Off, FPState::FPME_Off, FPState::FPS_Fast);
+  V = Builder.CreateFAdd(V, V);
+  CII = cast(V);
+  ASSERT_TRUE(isa(V));
+  ASSERT_TRUE(CII->getExceptionBehavior() == ConstrainedFPIntrinsic::ebIgnore);
+  ASSERT_TRUE(CII->getRoundingMode() == ConstrainedFPIntrinsic::rmToNearest);
+
+  fpState.updateBuilder(FPState::FPM_Off, FPState::FPME_Off,
+FPState::FPS_Strict);
+  V = Builder.CreateFAdd(V, V);
+  CII = cast(V);
+  ASSERT_TRUE(isa(V));
+  

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Documentation missing. All the blurb from patch description should be in `doc/`


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc created this revision.
mibintc added a reviewer: chandlerc.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Intel would like to contribute a patch to implement support for these Intel- 
and Microsoft -fp options.  This message is to describe the options and request 
feedback from the community.
-fp-model=[precise|strict|fast|except[-]] and -fp-speculation=[fast|strict|safe]

This contribution would dovetail with the llvm patch "Teach the IRBuilder about 
constrained fadd and friends" which is under review here, 
https://reviews.llvm.org/D53157/new/.  I have a patch ready to review that 
works with D53157 .  The motivation for 
providing these is that having a single option to control most basic FP options 
is better and easier to understand for users.

The companion llvm patch is available for review here: 
https://reviews.llvm.org/D62730 ; I made a couple small changes to D53157 
.

The option settings -fp-model=[precise|strict|fast|except] are supported by 
both ICC and CL. The fp-speculation option is supported only by ICC.  The CL 
and ICC -fp-model option is documented on these pages:

  
https://docs.microsoft.com/en-us/cpp/build/reference/fp-specify-floating-point-behavior?view=vs-2019
  
https://software.intel.com/en-us/cpp-compiler-developer-guide-and-reference-fp-model-fp

Currently, clang's default behavior corresponds to -fp-model=precise.  
Clang/llvm support for -fp-model=[strict|except] is being developed in the 
D53157  patch, and there is current llvm 
support for the fast settings by using the fast math flags llvm::FastMathFlags. 
 Note: the clang-cl wrapper to support Microsoft options has simplified support 
for these options by mapping /fp-model=except to ftrapping-math, fp-mdel=fast 
to ffast-math, fp-model=precise and fp-model=strict to fno-fast-math (see 
clang/Driver/CLCompatOptions.td).

According to the online options documentation, you can combine except[-] with 
[precise|strict|fast], but combining both fast and except is not a valid 
setting (the Driver will emit an error).

  precise - Disables optimizations that are not value-safe on floating-point 
data, although FP contraction is enabled.
  strict - Enables precise and except, disables contractions (FMA), and enables 
pragma stdc fenv_access.   
  fast - Equivalent to -ffast-math
  except/except- - Determines whether strict floating-point exception semantics 
are honored.

The ICC option -fp-speculation is described here,

  
https://software.intel.com/en-us/cpp-compiler-developer-guide-and-reference-fp-speculation-qfp-speculation

These are the meanings of the fp-speculation settings:

  fast - Tells the compiler to speculate on floating-point operations.  This is 
equivalent to “fpexcept.ignore” in the constrained intrinsics review D53157.
  strict - Tells the compiler to disable speculation on floating-point 
operations.  This is equivalent to “fpexcept.strict” in the constrained 
intrinsics review D53157.
  safe - Tells the compiler to disable speculation if there is a possibility 
that the speculation may cause a floating-point exception.  This is equivalent 
to “fpexcept.maytrap” in the constrained intrinsics review D53157.


Repository:
  rL LLVM

https://reviews.llvm.org/D62731

Files:
  include/clang/Basic/LangOptions.h
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  lib/CodeGen/CodeGenFunction.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/fpconstrained.c
  test/Driver/clang_f_opts.c

Index: test/Driver/clang_f_opts.c
===
--- test/Driver/clang_f_opts.c
+++ test/Driver/clang_f_opts.c
@@ -217,6 +217,11 @@
 // RUN: %clang -### -S -fexec-charset=iso-8859-1 -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-INPUT-CHARSET %s
 // CHECK-INVALID-INPUT-CHARSET: error: invalid value 'iso-8859-1' in '-fexec-charset=iso-8859-1'
 
+// RUN: %clang -### -S -fp-model=fast -fp-model=except -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-FAST-EXCEPT %s
+// RUN: %clang -### -S -fp-model=fast -fp-model=except -o /dev/null %s 2>&1 | FileCheck -check-prefix=CHECK-INVALID-FAST-EXCEPT %s
+// CHECK-INVALID-FAST-EXCEPT: error: invalid argument 'fp-model=fast' not allowed with 'fp-model=except'
+//
+
 // Test that we don't error on these.
 // RUN: %clang -### -S -Werror\
 // RUN: -falign-functions -falign-functions=2 -fno-align-functions\
Index: test/CodeGen/fpconstrained.c
===
--- /dev/null
+++ test/CodeGen/fpconstrained.c
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fp-model=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=STRICT
+// RUN: %clang_cc1 -fp-model=strict -fp-model-except -emit-llvm -o - %s | FileCheck %s