[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jeroen Dobbelaere via Phabricator via cfe-commits]

jeroen.dobbelaere added a comment.
Herald added a subscriber: dang.

Found some issue when looking at this code:  -ftrapping_math and 
-fno_trapping_math will never have effect




Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3155
+  if (Args.hasArg(OPT_ftrapping_math)) {
+Opts.setFPExceptionMode(LangOptions::FPE_Strict);
+  }

Calling 'Opts.setFPExceptionMode(xx)' here has no effect, as it will be 
overruled later on, on line 3174 !
Same is true on line 3159



Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

In D62731#1796962 , @AntonYudintsev 
wrote:

> I have found bug in clang-cl (win32 clang), related to recent inroduction of 
> ffp-exception-behavior.
>  Unfortunately, I don't have a working patch yet, and since LLVM bugtracker 
> registration is closed, I can not even submit a bug.
>
> So, if it is not a trouble for you, I will email the bug description here.
>
> Please let me know if it isn't appropriate. Bug description:
>  
>
> Windows: clang-cl is generating call to non-existing lib function for win32 
> with /fp:except option.
>  With recent ffp-exception-behavior=maytrap/strict, fp:except in clang-cl 
> became generate FPE aware code.
>
> But in case of floorf and ceilf it generates call to non-existing library 
> function.
>
> clang-cl.exe -m32 /Ox /fp:except testFloor.cpp /FA
>  testFloor.cpp:
>
>   #include 
>   float ret(float v) {  return floorf(v); }
>
>
> resulting assember:
>
>   pusheax
>   movssxmm0, dword ptr [esp + 8]
>   movssdword ptr [esp], xmm0
>   call_floorf #no such function!!!
>   popeax
>   ret
>   
>
> Expected behaviour:
>
> there is no floorf lib function. Like with cosf and other math functions, 
> floorf in MSVC is implemented as inline function.
>
> So, it should be call to _floor (with apropriate conversion first).


Hopefully fixed by 53ee806d93e8d2371726ec5ce59b0e68b309c258 



Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Eric Christopher via cfe-commits]

You can sign up for an account by emailing the admin account.

On Thu, Dec 26, 2019, 5:04 PM Anton Yudintsev via Phabricator <
revi...@reviews.llvm.org> wrote:

> AntonYudintsev added a comment.
>
> I have found bug in clang-cl (win32 clang), related to recent inroduction
> of ffp-exception-behavior.
> Unfortunately, I don't have a working patch yet, and since LLVM bugtracker
> registration is closed, I can not even submit a bug.
>
> So, if it is not a trouble for you, I will email the bug description here.
>
> Please let me know if it isn't appropriate. Bug description:
> 
>
> Windows: clang-cl is generating call to non-existing lib function for
> win32 with /fp:except option.
> With recent ffp-exception-behavior=maytrap/strict, fp:except in clang-cl
> became generate FPE aware code.
>
> But in case of floorf and ceilf it generates call to non-existing library
> function.
>
> clang-cl.exe -m32 /Ox /fp:except testFloor.cpp /FA
> testFloor.cpp:
>
>   #include 
>   float ret(float v) {  return floorf(v); }
>
> resulting assember:
>
>   pusheax
>   movssxmm0, dword ptr [esp + 8]
>   movssdword ptr [esp], xmm0
>   call_floorf #no such function!!!
>   popeax
>   ret
>
> Expected behaviour:
>
> there is no floorf lib function. Like with cosf and other math functions,
> floorf in MSVC is implemented as inline function.
>
> So, it should be call to _floor (with apropriate conversion first).
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> 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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Anton Yudintsev via Phabricator via cfe-commits]

AntonYudintsev added a comment.

I have found bug in clang-cl (win32 clang), related to recent inroduction of 
ffp-exception-behavior.
Unfortunately, I don't have a working patch yet, and since LLVM bugtracker 
registration is closed, I can not even submit a bug.

So, if it is not a trouble for you, I will email the bug description here.

Please let me know if it isn't appropriate. Bug description:


Windows: clang-cl is generating call to non-existing lib function for win32 
with /fp:except option.
With recent ffp-exception-behavior=maytrap/strict, fp:except in clang-cl became 
generate FPE aware code.

But in case of floorf and ceilf it generates call to non-existing library 
function.

clang-cl.exe -m32 /Ox /fp:except testFloor.cpp /FA
testFloor.cpp:

  #include 
  float ret(float v) {  return floorf(v); }

resulting assember:

  pusheax
  movssxmm0, dword ptr [esp + 8]
  movssdword ptr [esp], xmm0
  call_floorf #no such function!!!
  popeax
  ret

Expected behaviour:

there is no floorf lib function. Like with cosf and other math functions, 
floorf in MSVC is implemented as inline function.

So, it should be call to _floor (with apropriate conversion first).


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

In D62731#1790211 , @rupprecht wrote:

> In D62731#1789122 , @rupprecht wrote:
>
> > In D62731#1788902 , 
> > @andrew.w.kaylor wrote:
> >
> > > In D62731#1788838 , @rupprecht 
> > > wrote:
> > >
> > > > It seems the discussion of whether or not this is incomplete died out 
> > > > -- I'd prefer to assume it is incomplete if there is no consensus. 
> > > > Mailed D71635  to rename 
> > > > `-frounding-math` to `-fexperimental-rounding-math`.
> > > >
> > > > Alternatively we could remove the warning. I still don't see a good 
> > > > argument for the middle ground of having it called `-frounding-math` 
> > > > but also generate a warning.
> > >
> > >
> > > It's definitely incomplete but the results will not be any worse than you 
> > > get when -frounding-math is ignored.
> > >
> > > My preference would be to change the text of the warning that is issued 
> > > but allow -frounding-math to be enabled by this commit without requiring 
> > > an additional option.
> >
> >
> > If other reviewers agree, then let's just remove the warning. I can send a 
> > patch tomorrow unless someone else wants to do that.
>
>
> Mailed D71671  to do this.
>
> If neither patch is acceptable, then I would like to revert this commit 
> instead, as we are having issues with this patch.


I think we should stick with this patch and remove the warning like you 
proposed in D71671 


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D62731#1789122 , @rupprecht wrote:

> In D62731#1788902 , @andrew.w.kaylor 
> wrote:
>
> > In D62731#1788838 , @rupprecht 
> > wrote:
> >
> > > It seems the discussion of whether or not this is incomplete died out -- 
> > > I'd prefer to assume it is incomplete if there is no consensus. Mailed 
> > > D71635  to rename `-frounding-math` to 
> > > `-fexperimental-rounding-math`.
> > >
> > > Alternatively we could remove the warning. I still don't see a good 
> > > argument for the middle ground of having it called `-frounding-math` but 
> > > also generate a warning.
> >
> >
> > It's definitely incomplete but the results will not be any worse than you 
> > get when -frounding-math is ignored.
> >
> > My preference would be to change the text of the warning that is issued but 
> > allow -frounding-math to be enabled by this commit without requiring an 
> > additional option.
>
>
> If other reviewers agree, then let's just remove the warning. I can send a 
> patch tomorrow unless someone else wants to do that.


Mailed D71671  to do this.

If neither patch is acceptable, then I would like to revert this commit 
instead, as we are having issues with this patch.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht closed this revision.
rupprecht added a comment.

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

> In D62731#1788838 , @rupprecht wrote:
>
> > It seems the discussion of whether or not this is incomplete died out -- 
> > I'd prefer to assume it is incomplete if there is no consensus. Mailed 
> > D71635  to rename `-frounding-math` to 
> > `-fexperimental-rounding-math`.
> >
> > Alternatively we could remove the warning. I still don't see a good 
> > argument for the middle ground of having it called `-frounding-math` but 
> > also generate a warning.
>
>
> It's definitely incomplete but the results will not be any worse than you get 
> when -frounding-math is ignored.
>
> My preference would be to change the text of the warning that is issued but 
> allow -frounding-math to be enabled by this commit without requiring an 
> additional option.


If other reviewers agree, then let's just remove the warning. I can send a 
patch tomorrow unless someone else wants to do that.

> I would also very much like to see this patch re-committed. It's currently in 
> the "approved" state. If anyone objects to this being committed, please use 
> the "request changes" action to indicate this.

It is already re-committed. 7f9b5138470db1dc58f3bc05631284c653c9ed7a 
 reapplied 
it, but IIUC it was not closed in phabricator due to leading whitespace in the 
commit message:

  Reapply af57dbf12e54 "Add support for options -frounding-math, 
ftrapping-math, -ffp-model=, and -ffp-exception-behavior="
  ...
  Differential Revision: https://reviews.llvm.org/D62731

The "Differential" needs to be the first thing, whitespace cannot come before 
it.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added a comment.

In D62731#1788838 , @rupprecht wrote:

> It seems the discussion of whether or not this is incomplete died out -- I'd 
> prefer to assume it is incomplete if there is no consensus. Mailed D71635 
>  to rename `-frounding-math` to 
> `-fexperimental-rounding-math`.
>
> Alternatively we could remove the warning. I still don't see a good argument 
> for the middle ground of having it called `-frounding-math` but also generate 
> a warning.


It's definitely incomplete but the results will not be any worse than you get 
when -frounding-math is ignored.

My preference would be to change the text of the warning that is issued but 
allow -frounding-math to be enabled by this commit without requiring an 
additional option.

I would also very much like to see this patch re-committed. It's currently in 
the "approved" state. If anyone objects to this being committed, please use the 
"request changes" action to indicate this.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

It seems the discussion of whether or not this is incomplete died out -- I'd 
prefer to assume it is incomplete if there is no consensus. Mailed D71635 
 to rename `-frounding-math` to 
`-fexperimental-rounding-math`.

Alternatively we could remove the warning. I still don't see a good argument 
for the middle ground of having it called `-frounding-math` but also generate a 
warning.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

In D62731#1784491 , @pengfei wrote:

> >> It's good that people are looking at achieving better modeling for the x86 
> >> backend, but we need to have a plan that doesn't require heroic effort 
> >> just to get basic correctness.
> > 
> > Do you mean in the backend? If so, I don't think that's possible. The 
> > backends just don't have any sort of feature that could be used to get 
> > conservatively correct behavior for cheap the way intrinsics give it to us 
> > in the middle end. Once you go into instruction selection things get very 
> > low level in a hurry.
>
> I'm looking for simple ways to modeling X86 intrinsics, but haven't find 
> better one than modeling it one by one.
>
> > I would suggest that we need a function/call attribute roughly on the level 
> > of `readonly` / `readnone`, maybe `readfponly`, that says that a function 
> > has no side-effects and no dependencies on anything *except* the FP state.
>
> Do you mean mark it at the declaration of intrinsics? Is it reasonable to 
> mark `except` on dependent intrinsics?
>
> > Basic queries like `Instruction::mayReadMemory()` that are supposed to be 
> > used generically in code-motion transforms would then return true for calls 
> > marked that way only if they're FP-constrained functions.
>
> Middle end or back end? I think in middle end you may need to change all 
> releated passes to get such information to prevent optimization. And in back 
> end, I think we can simply chain intrinsics marked `except` with other FP 
> nodes like what common code doing.


We don't want to do this all the time. So we need a new property for the 
intrinsics that means we should prevent code motion in the middle end when the 
calling function has the strictfp attribute. Similarly SelectionDAGBuilder 
should use INTRINSIC_W_CHAIN instead of INTRINSIC_WO_CHAIN for any of these 
intrinsics when strictp is enabled.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Pengfei Wang via Phabricator via cfe-commits]

pengfei added a comment.

>> It's good that people are looking at achieving better modeling for the x86 
>> backend, but we need to have a plan that doesn't require heroic effort just 
>> to get basic correctness.
> 
> Do you mean in the backend? If so, I don't think that's possible. The 
> backends just don't have any sort of feature that could be used to get 
> conservatively correct behavior for cheap the way intrinsics give it to us in 
> the middle end. Once you go into instruction selection things get very low 
> level in a hurry.

I'm looking for simple ways to modeling X86 intrinsics, but haven't find better 
one than modeling it one by one.

> I would suggest that we need a function/call attribute roughly on the level 
> of `readonly` / `readnone`, maybe `readfponly`, that says that a function has 
> no side-effects and no dependencies on anything *except* the FP state.

Do you mean mark it at the declaration of intrinsics? Is it reasonable to mark 
`except` on dependent intrinsics?

> Basic queries like `Instruction::mayReadMemory()` that are supposed to be 
> used generically in code-motion transforms would then return true for calls 
> marked that way only if they're FP-constrained functions.

Middle end or back end? I think in middle end you may need to change all 
releated passes to get such information to prevent optimization. And in back 
end, I think we can simply chain intrinsics marked `except` with other FP nodes 
like what common code doing.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:444
 
+def warn_drv_experimental_fp_control_incomplete_opt : Warning<
+  "Support for floating point control option %0 is incomplete and 
experimental">,

rupprecht wrote:
> andrew.w.kaylor wrote:
> > rupprecht wrote:
> > > mibintc wrote:
> > > > @kpn thought it would be a good idea to add a Warning that the 
> > > > implementation of float control is experimental and partially 
> > > > implemented.  That's what this is for.
> > > Instead of adding a warning, I'd like to propose `-frounding-math` not be 
> > > enabled unless an additional flag (e.g. `-fexperimental-float-control`) 
> > > is passed. Or maybe this feature should be called 
> > > `-f[no-]experimental-rounding-math` instead.
> > > 
> > > There are plenty of builds that are already specifying `-frounding-math` 
> > > (e.g. they also support building w/ a compiler such as gcc that 
> > > implements this), and adding this experimental/incomplete implementation 
> > > is a surprise to those builds.
> > > 
> > > If I'm wrong and it's completely safe to ignore the warning (maybe it's 
> > > incomplete but not in any unsafe way), we should just not have it at all.
> > You raise an interesting point about people who might be using 
> > -frounding-math already. However, if they are using this flag because they 
> > also sometimes build with a compiler such as gcc that supports the flag, 
> > they are currently getting incorrect behavior from clang. Without this 
> > patch, clang silently ignores the option and the optimizer silently ignores 
> > the fact that the program may be changing the rounding mode dynamically. 
> > The user may or may not be aware of this.
> > 
> > With this patch such a user is likely to observe two effects: (1) their 
> > code will suddenly get slower, and (2) it will probably start behaving 
> > correctly with regard to rounding mode changes. The rounding behavior will 
> > definitely not get worse. I think the warning is useful as an indication 
> > that something has changed. I don't think requiring an additional option 
> > should be necessary.
> > However, if they are using this flag because they also sometimes build with 
> > a compiler such as gcc that supports the flag, they are currently getting 
> > incorrect behavior from clang
> 
> Incorrect, yes, but also likely valid behavior. "experimental" seems to imply 
> a miscompile when using this option should not be unexpected.
> 
> As I suggested before: if I'm wrong, and this behavior is only going to make 
> the code more correct (as you suggest), can we remove the warning that this 
> must be ack'd explicitly by adding `-Wno-experimental-float-control` to 
> builds? I don't understand the motivation for the warning.
The "experimental" code won't be incorrect in any way that the code generated 
when we ignore the option is. The things that have been implemented will work 
correctly. The things that are not implemented will have the potential to 
disregard runtime changes to the rounding mode. Currently, dynamic changes to 
the rounding mode always have the potential of being ignored.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added a comment.

In D62731#1784225 , @rjmccall wrote:

> ... The big problem is that intrinsic calls are not arbitrary code: the vast 
> majority of intrinsics (e.g. all the ARM vector intrinsics, many of which can 
> be floating-point) are marked `IntrNoMem` (which I believe corresponds to 
> `readnone`), which means calls to those intrinsics can be reordered with 
> other code whether or not that code has arbitrary side-effects.


Oh, you're right. With the constrained intrinsics we are currently handling 
that by using IntrInaccessibleMemOnly as a proxy for access to the FP 
environment, but that's stronger than we'd want for architecture-specific 
intrinsics in the default case. We have talked about an fpenv-specific 
attribute, but nothing has been done. So, I guess that does leave us in the 
situation where rounding controls might not be correctly respected if 
target-specific intrinsics are used.

> It's good that people are looking at achieving better modeling for the x86 
> backend, but we need to have a plan that doesn't require heroic effort just 
> to get basic correctness.

Do you mean in the backend? If so, I don't think that's possible. The backends 
just don't have any sort of feature that could be used to get conservatively 
correct behavior for cheap the way intrinsics give it to us in the middle end. 
Once you go into instruction selection things get very low level in a hurry.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I believe your analysis on the second point is unfortunately missing half the 
problem.  Functions like `fesetround` will be treated as having arbitrary 
side-effects by default, which mean arbitrary code can't be reordered with 
calls to them.  It'd be good to model that more precisely — to flag that the 
*only* side-effect they have is changing the FP state — but that's not really 
the big problem; that's just enabling better optimization by e.g. allowing them 
to be reordered with non-FP code.  The big problem is that intrinsic calls are 
not arbitrary code: the vast majority of intrinsics (e.g. all the ARM vector 
intrinsics, many of which can be floating-point) are marked `IntrNoMem` (which 
I believe corresponds to `readnone`), which means calls to those intrinsics can 
be reordered with other code whether or not that code has arbitrary 
side-effects.  It's good that people are looking at achieving better modeling 
for the x86 backend, but we need to have a plan that doesn't require heroic 
effort just to get basic correctness.

I would suggest that we need a function/call attribute roughly on the level of 
`readonly` / `readnone`, maybe `readfponly`, that says that a function has no 
side-effects and no dependencies on anything *except* the FP state.  Basic 
queries like `Instruction::mayReadMemory()` that are supposed to be used 
generically in code-motion transforms would then return true for calls marked 
that way only if they're FP-constrained functions.  So outside of an 
FP-constrained function we'd preserve optimizability, and inside one we'd be 
appropriately conservative.  The generic backend could similarly just default 
to treating those intrinsic calls as having side-effects in FP-constrained 
functions, and there'd just be some way for a backend to opt out of that when 
it provides precise modeling of FP state.  It'd then be a fairly 
straightforward change to go through the target intrinsic tables and mark which 
ones have dependencies on FP state.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added a comment.

In D62731#1782912 , @rjmccall wrote:

> Hmm.  The target-specific intrinsics thing does concern me, since I assume 
> many targets have a bunch of vector intrinsics that may be sensitive to 
> rounding.  Do we have an idea of how we'd fix that?  If it's a short-term 
> incorrectness that we have a plan to fix, I don't mind the risk, but if we 
> don't know how we'd even start to address it...


I see two potential problem cases with the target-specific intrinsics:

1. Some optimization pass recognizes the intrinisic and uses its semantics to 
perform some optimization such as constant folding
2. Some optimization performs code motion that moves the intrinsic (or, in the 
backend, the instruction it represents) across an operation that changes the 
rounding mode

I don't know if there are any instances of the first case in the public 
repository. Downstream users could be doing it. Those will need special 
handling if they exist (checking for the the strictfp attribute).

The second case should be handled in IR by fesetround() or other such 
intrinsics being marked as having side effects. It's possible that there are 
target-specific intrinsics to change the rounding mode that aren't marked as 
having side effects, but if so that's simply a bug. The other part of this is 
that the intrinsic might be lowered to MC and the MC instructions in a way that 
neglects rounding mode. Many targets have instructions with forms that take an 
explicit rounding mode argument and the backends may be using that with the 
default rounding mode. I am not aware of any such case, but it's definitely 
possible.

Finally, our design for handling strict fp mode in the backends is that 
rounding mode control will be handled by explicitly modeling the dependency 
between the relevant control registers and instructions that implicitly use the 
rounding mode controled by those registers. X86 only recently started doing 
this. There may be other backends that have not implemented it. Some may never 
do so.

I don't have a strong preference about what to do with the warning. I have a 
slight preference for replacing the existing warning with a more specific 
warning saying that floating math support is a work in progress. Eventually we 
need a way for backends to indicate that they believe their support is complete.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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

> In D62731#1782762 , @rjmccall wrote:
>
> > Currently we emit a warning if you use `-frounding-math`, saying that the 
> > option is ignored.  That should have alerted users that they're not getting 
> > the correct behavior now.  This patch removes the warning and (IIUC) 
> > implements the correct behavior but is over-conservative.  If that's 
> > correct, I think that's acceptable and we don't need an "experimental" flag 
> > or a warning.
>
>
> Oh, I didn't realize we were already warning about that. In theory, we should 
> handle rounding math correctly with this change. It's possible we've missed 
> some things, but I suppose that's always true. I think there are a few 
> general intrinsics left that need constrained versions but don't have them, 
> and we don't have any solution yet for target-specific intrinsics. If any of 
> those have special handling that assumes the default rounding mode we will 
> get it wrong. I don't think most users would be likely to encounter a problem.


Hmm.  The target-specific intrinsics thing does concern me, since I assume many 
targets have a bunch of vector intrinsics that may be sensitive to rounding.  
Do we have an idea of how we'd fix that?  If it's a short-term incorrectness 
that we have a plan to fix, I don't mind the risk, but if we don't know how 
we'd even start to address it...


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added a comment.

In D62731#1782762 , @rjmccall wrote:

> Currently we emit a warning if you use `-frounding-math`, saying that the 
> option is ignored.  That should have alerted users that they're not getting 
> the correct behavior now.  This patch removes the warning and (IIUC) 
> implements the correct behavior but is over-conservative.  If that's correct, 
> I think that's acceptable and we don't need an "experimental" flag or a 
> warning.


Oh, I didn't realize we were already warning about that. In theory, we should 
handle rounding math correctly with this change. It's possible we've missed 
some things, but I suppose that's always true. I think there are a few general 
intrinsics left that need constrained versions but don't have them, and we 
don't have any solution yet for target-specific intrinsics. If any of those 
have special handling that assumes the default rounding mode we will get it 
wrong. I don't think most users would be likely to encounter a problem.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Currently we emit a warning if you use `-frounding-math`, saying that the 
option is ignored.  That should have alerted users that they're not getting the 
correct behavior now.  This patch removes the warning and (IIUC) implements the 
correct behavior but is over-conservative.  If that's correct, I think that's 
acceptable and we don't need an "experimental" flag or a warning.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:444
 
+def warn_drv_experimental_fp_control_incomplete_opt : Warning<
+  "Support for floating point control option %0 is incomplete and 
experimental">,

andrew.w.kaylor wrote:
> rupprecht wrote:
> > mibintc wrote:
> > > @kpn thought it would be a good idea to add a Warning that the 
> > > implementation of float control is experimental and partially 
> > > implemented.  That's what this is for.
> > Instead of adding a warning, I'd like to propose `-frounding-math` not be 
> > enabled unless an additional flag (e.g. `-fexperimental-float-control`) is 
> > passed. Or maybe this feature should be called 
> > `-f[no-]experimental-rounding-math` instead.
> > 
> > There are plenty of builds that are already specifying `-frounding-math` 
> > (e.g. they also support building w/ a compiler such as gcc that implements 
> > this), and adding this experimental/incomplete implementation is a surprise 
> > to those builds.
> > 
> > If I'm wrong and it's completely safe to ignore the warning (maybe it's 
> > incomplete but not in any unsafe way), we should just not have it at all.
> You raise an interesting point about people who might be using 
> -frounding-math already. However, if they are using this flag because they 
> also sometimes build with a compiler such as gcc that supports the flag, they 
> are currently getting incorrect behavior from clang. Without this patch, 
> clang silently ignores the option and the optimizer silently ignores the fact 
> that the program may be changing the rounding mode dynamically. The user may 
> or may not be aware of this.
> 
> With this patch such a user is likely to observe two effects: (1) their code 
> will suddenly get slower, and (2) it will probably start behaving correctly 
> with regard to rounding mode changes. The rounding behavior will definitely 
> not get worse. I think the warning is useful as an indication that something 
> has changed. I don't think requiring an additional option should be necessary.
> However, if they are using this flag because they also sometimes build with a 
> compiler such as gcc that supports the flag, they are currently getting 
> incorrect behavior from clang

Incorrect, yes, but also likely valid behavior. "experimental" seems to imply a 
miscompile when using this option should not be unexpected.

As I suggested before: if I'm wrong, and this behavior is only going to make 
the code more correct (as you suggest), can we remove the warning that this 
must be ack'd explicitly by adding `-Wno-experimental-float-control` to builds? 
I don't understand the motivation for the warning.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:444
 
+def warn_drv_experimental_fp_control_incomplete_opt : Warning<
+  "Support for floating point control option %0 is incomplete and 
experimental">,

rupprecht wrote:
> mibintc wrote:
> > @kpn thought it would be a good idea to add a Warning that the 
> > implementation of float control is experimental and partially implemented.  
> > That's what this is for.
> Instead of adding a warning, I'd like to propose `-frounding-math` not be 
> enabled unless an additional flag (e.g. `-fexperimental-float-control`) is 
> passed. Or maybe this feature should be called 
> `-f[no-]experimental-rounding-math` instead.
> 
> There are plenty of builds that are already specifying `-frounding-math` 
> (e.g. they also support building w/ a compiler such as gcc that implements 
> this), and adding this experimental/incomplete implementation is a surprise 
> to those builds.
> 
> If I'm wrong and it's completely safe to ignore the warning (maybe it's 
> incomplete but not in any unsafe way), we should just not have it at all.
You raise an interesting point about people who might be using -frounding-math 
already. However, if they are using this flag because they also sometimes build 
with a compiler such as gcc that supports the flag, they are currently getting 
incorrect behavior from clang. Without this patch, clang silently ignores the 
option and the optimizer silently ignores the fact that the program may be 
changing the rounding mode dynamically. The user may or may not be aware of 
this.

With this patch such a user is likely to observe two effects: (1) their code 
will suddenly get slower, and (2) it will probably start behaving correctly 
with regard to rounding mode changes. The rounding behavior will definitely not 
get worse. I think the warning is useful as an indication that something has 
changed. I don't think requiring an additional option should be necessary.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:444
 
+def warn_drv_experimental_fp_control_incomplete_opt : Warning<
+  "Support for floating point control option %0 is incomplete and 
experimental">,

mibintc wrote:
> @kpn thought it would be a good idea to add a Warning that the implementation 
> of float control is experimental and partially implemented.  That's what this 
> is for.
Instead of adding a warning, I'd like to propose `-frounding-math` not be 
enabled unless an additional flag (e.g. `-fexperimental-float-control`) is 
passed. Or maybe this feature should be called 
`-f[no-]experimental-rounding-math` instead.

There are plenty of builds that are already specifying `-frounding-math` (e.g. 
they also support building w/ a compiler such as gcc that implements this), and 
adding this experimental/incomplete implementation is a surprise to those 
builds.

If I'm wrong and it's completely safe to ignore the warning (maybe it's 
incomplete but not in any unsafe way), we should just not have it at all.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

In D62731#1780597 , @cameron.mcinally 
wrote:

> I don't see a problem with this, but it would be nice to make the 
> `-f[no-]trapping-math` command line option work. GNU compatibility is good.


Thanks Cameron, I'll go that way


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht marked an inline comment as done.
rupprecht added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:13047
+  F->setUsesFPIntrin(true);
+  printf("Enclosing function uses fp intrinsics\n");
+}

rupprecht wrote:
> Looks like this is leftover debugging? I'm seeing log spam compiling some 
> files -- this message repeated hundreds of times. I'll go ahead and create a 
> patch that nukes this.
Sorry for the noise, looks like f4a7d5659df7cb56c1baa34a39e9fe2639472741 
already did this.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:13047
+  F->setUsesFPIntrin(true);
+  printf("Enclosing function uses fp intrinsics\n");
+}

Looks like this is leftover debugging? I'm seeing log spam compiling some files 
-- this message repeated hundreds of times. I'll go ahead and create a patch 
that nukes this.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Cameron McInally via Phabricator via cfe-commits]

cameron.mcinally added a comment.

In D62731#1779872 , @mibintc wrote:

> In D62731#1778597 , 
> @michele.scandale wrote:
>
> > >> I've noticed you removed the change for `CompilerInvocation.cpp` about 
> > >> the initialization of the codegen option `NoTrappingMath`. Was that an 
> > >> accident?
>
>
> Thanks again Michele.  I'd like to get rid of Opts.NoTrappingMath, but I 
> haven't been bold enough yet.


I don't see a problem with this, but it would be nice to make the 
`-f[no-]trapping-math` command line option work. GNU compatibility is good.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

In D62731#1778597 , @michele.scandale 
wrote:

> >> I've noticed you removed the change for `CompilerInvocation.cpp` about the 
> >> initialization of the codegen option `NoTrappingMath`. Was that an 
> >> accident?


Thanks again Michele.  I'd like to get rid of Opts.NoTrappingMath, but I 
haven't been bold enough yet. NoTrappingMath is not expressive enough because 
it can hold only 2 values, whereas the Exception behavior can be ignore, strict 
or maytrap. So I'd get rid of that Opts field, and the only place where I see 
it actually being used is in llvm/lib/Target/ARM/ARMAsmPrinter.cpp and the 
change in this patch doesn't seem to affect the ARM logic so I think if I got 
rid of it, it would be OK. All the other instances of the string are in llvm 
test cases.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added a comment.

In D62731#1775046 , @mibintc wrote:

> In D62731#1773854 , 
> @michele.scandale wrote:
>
> > I've noticed you removed the change for `CompilerInvocation.cpp` about the 
> > initialization of the codegen option `NoTrappingMath`. Was that an accident?
>
>
> I checked the old and new version of the patch and it seems like 
> initialization of NoTrappingMath is unchanged, the definition of the option 
> has it default to 0, and CompilerInvocation.cpp sets it like this in 
> ParseLangArgs, was it something else you were looking at?
>
>   Opts.NoTrappingMath = Args.hasArg(OPT_fno_trapping_math);


In the driver code you don't always pass `-fno-trapping-math`, therefore when 
when the compiler setup the CodeGen options in `ParseCodeGenArgs` you will end 
up executing `Opts.NoTrappingMath = Args.hasArg(OPT_fno_trapping_math);` hence 
you will have that `Opt.NoTrappingMath = false`. This is inconsistent with the 
state of the compiler driver where no-trapping-math is enabled default.

If you want to keep the default of the CC1 different than the default of the 
compiler driver, that's fine to me, but in that case the compiler driver needs 
to pass `-fno-trapping-math` to the CC1.
If we want the same new default, then the logic in `ParseCodeGenArgs` must be 
updated.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

In D62731#1773854 , @michele.scandale 
wrote:

> I've noticed you removed the change for `CompilerInvocation.cpp` about the 
> initialization of the codegen option `NoTrappingMath`. Was that an accident?


I checked the old and new version of the patch and it seems like initialization 
of NoTrappingMath is unchanged, the definition of the option has it default to 
0, and CompilerInvocation.cpp sets it like this in ParseLangArgs, was it 
something else you were looking at?
 Opts.NoTrappingMath = Args.hasArg(OPT_fno_trapping_math);


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added a comment.

I've noticed you removed the change for `CompilerInvocation.cpp` about the 
initialization of the codegen option `NoTrappingMath`. Was that an accident?


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

I fixed the lit test problem and pushed it again

commit 7f9b5138470db1dc58f3bc05631284c653c9ed7a 

Author: Melanie Blower 


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

> It appears that only the 1st failure there is the fault of this patch.  The 
> 2nd seems to have come from some openmp patch (that didn't consider dso_local 
> on windows).
> 
> The first (fpconstrained.cpp) likely just needs the check-lines to NOT 
> explicitly say the %4/%5 and capture the loads of those with a wildcard 
> instead.

Yes, sorry, I pasted the wrong link. http://45.33.8.238/win/3402/step_6.txt is 
the one for this commit, and it has just one of the two failures.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D62731#1769532 , @thakis wrote:

> The tests fail on Windows, http://45.33.8.238/win/3405/step_6.txt
>
> Please take a look, and if it takes a while to fix please revert while you 
> investigte.


It appears that only the 1st failure there is the fault of this patch.  The 2nd 
seems to have come from some openmp patch (that didn't consider dso_local on 
windows).

The first (fpconstrained.cpp) likely just needs the check-lines to NOT 
explicitly say the %4/%5 and capture the loads of those with a wildcard instead.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

I reverted it again because build break on windows


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

The tests fail on Windows, http://45.33.8.238/win/3405/step_6.txt

Please take a look, and if it takes a while to fix please revert while you 
investigte.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked an inline comment as done.
mibintc added a comment.

I've pushed the updated patch,
commit cdbed2dd856c14687efd741c2d8321686102acb8 



Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2345
+<< "-ffp-contract=fast";
+optID = options::OPT_ffp_contract;
+FPModel = Val;

mibintc wrote:
> michele.scandale wrote:
> > Here the state of the floating point options seems unchanged except for 
> > `FPContract`. If I run `clang -ffp-model=fast -ffp-model=precise`, I would 
> > expect the state of the floating point options to match the one of 
> > `-fno-fast-math` except for `FPContract` which you want to be set to "fast".
> > 
> > I think you might need to replicate the reset for all the option here as 
> > well, so at this point I don't know how much worth is to use the optID 
> > reset trick for the "fast" case only.
> @michele.scandale Thanks for your helpful review, I think I fixed the things 
> that you remarked on. I also added a test case for the assertion fail that 
> you saw.
Thanks!


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

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

kpn added inline comments.



Comment at: llvm/include/llvm/IR/IRBuilder.h:268
   I->addAttribute(AttributeList::FunctionIndex, Attribute::StrictFP);
-setConstrainedFPFunctionAttr();
   }

mibintc wrote:
> @kpn I got rid of this line because the function attribute is being set in 
> CodeGen
Makes sense.



Comment at: llvm/unittests/IR/IRBuilderTest.cpp:186
   Builder.setIsFPConstrained(true);
+  auto Parent = BB->getParent();
+  Parent->addFnAttr(Attribute::StrictFP);

mibintc wrote:
> @kpn I changed the test to create the function attribute a priori since it 
> will be set in CodeGen before passing to IRBuilder
Right, of course.

I'm not going to quibble over the use of auto. It's fine I think.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

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

I added inline comments describing what I did in this version of the patch to 
address the bug https://bugs.llvm.org/show_bug.cgi?id=44048




Comment at: clang/include/clang/AST/DeclBase.h:1539
+/// Indicates if the function uses Floating Point Constrained Intrinsics
+uint64_t UsesFPIntrin : 1;
   };

This corresponds to "strictfp" LLVM attribute. I add this here because I want 
to collect the information during Sema and set the attribute during CodeGen. 
The next thing I want to do is to add support for modifying float_control via 
pragma within function bodies (enable floating point control at the block 
level).  If I wasn't preparing to support floating_control via statement-level 
pragma then setting the bit could be accomplished entirely within CodeGen.



Comment at: clang/include/clang/AST/DeclBase.h:1560
 /// NumFunctionDeclBitfields or CXXConstructorDeclBitfields.
-uint64_t NumCtorInitializers : 23;
+uint64_t NumCtorInitializers : 22;
 uint64_t IsInheritingConstructor : 1;

Need to adjust the number of bits here, because it's at the threshold of 
overrunning 64 bits.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:444
 
+def warn_drv_experimental_fp_control_incomplete_opt : Warning<
+  "Support for floating point control option %0 is incomplete and 
experimental">,

@kpn thought it would be a good idea to add a Warning that the implementation 
of float control is experimental and partially implemented.  That's what this 
is for.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2345
+<< "-ffp-contract=fast";
+optID = options::OPT_ffp_contract;
+FPModel = Val;

michele.scandale wrote:
> Here the state of the floating point options seems unchanged except for 
> `FPContract`. If I run `clang -ffp-model=fast -ffp-model=precise`, I would 
> expect the state of the floating point options to match the one of 
> `-fno-fast-math` except for `FPContract` which you want to be set to "fast".
> 
> I think you might need to replicate the reset for all the option here as 
> well, so at this point I don't know how much worth is to use the optID reset 
> trick for the "fast" case only.
@michele.scandale Thanks for your helpful review, I think I fixed the things 
that you remarked on. I also added a test case for the assertion fail that you 
saw.



Comment at: clang/test/CodeGen/fpconstrained.cpp:10
+
+  template 
+  class  {

This is the test case from the bug report (null deref/segfault/in IRBuilder)



Comment at: llvm/include/llvm/IR/IRBuilder.h:268
   I->addAttribute(AttributeList::FunctionIndex, Attribute::StrictFP);
-setConstrainedFPFunctionAttr();
   }

@kpn I got rid of this line because the function attribute is being set in 
CodeGen



Comment at: llvm/unittests/IR/IRBuilderTest.cpp:186
   Builder.setIsFPConstrained(true);
+  auto Parent = BB->getParent();
+  Parent->addFnAttr(Attribute::StrictFP);

@kpn I changed the test to create the function attribute a priori since it will 
be set in CodeGen before passing to IRBuilder


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

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

This commit was reverted in 30e7ee3c4bac 
 because a 
null deref error occurred in IRBuilder.h when setting strictfp attribute, see 
https://bugs.llvm.org/show_bug.cgi?id=44048 for information about that bug.
This patch moves setting strictfp from IRBuilder into clang/CodeGen. Also 
addresses some code review comments that were received after the revert.  I'll 
add some inline comments next.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/Decl.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CodeGen/fpconstrained.c
  clang/test/CodeGen/fpconstrained.cpp
  clang/test/Driver/clang_f_opts.c
  clang/test/Driver/fast-math.c
  clang/test/Driver/fp-model.c
  llvm/include/llvm/IR/IRBuilder.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/unittests/IR/IRBuilderTest.cpp

Index: llvm/unittests/IR/IRBuilderTest.cpp
===
--- llvm/unittests/IR/IRBuilderTest.cpp
+++ llvm/unittests/IR/IRBuilderTest.cpp
@@ -183,6 +183,8 @@
   // See if we get constrained intrinsics instead of non-constrained
   // instructions.
   Builder.setIsFPConstrained(true);
+  auto Parent = BB->getParent();
+  Parent->addFnAttr(Attribute::StrictFP);
 
   V = Builder.CreateFAdd(V, V);
   ASSERT_TRUE(isa(V));
@@ -233,7 +235,8 @@
   AttributeSet CallAttrs = II->getAttributes().getFnAttributes();
   EXPECT_EQ(CallAttrs.hasAttribute(Attribute::StrictFP), true);
 
-  // Verify attributes on the containing function are created automatically.
+  // Verify attributes on the containing function are created when requested.
+  Builder.setConstrainedFPFunctionAttr();
   AttributeList Attrs = BB->getParent()->getAttributes();
   AttributeSet FnAttrs = Attrs.getFnAttributes();
   EXPECT_EQ(FnAttrs.hasAttribute(Attribute::StrictFP), true);
Index: llvm/include/llvm/Target/TargetOptions.h
===
--- llvm/include/llvm/Target/TargetOptions.h
+++ llvm/include/llvm/Target/TargetOptions.h
@@ -107,7 +107,7 @@
   public:
 TargetOptions()
 : PrintMachineCode(false), UnsafeFPMath(false), NoInfsFPMath(false),
-  NoNaNsFPMath(false), NoTrappingFPMath(false),
+  NoNaNsFPMath(false), NoTrappingFPMath(true),
   NoSignedZerosFPMath(false),
   HonorSignDependentRoundingFPMathOption(false), NoZerosInBSS(false),
   GuaranteedTailCallOpt(false), StackSymbolOrdering(true),
Index: llvm/include/llvm/IR/IRBuilder.h
===
--- llvm/include/llvm/IR/IRBuilder.h
+++ llvm/include/llvm/IR/IRBuilder.h
@@ -265,7 +265,6 @@
   void setConstrainedFPCallAttr(CallInst *I) {
 if (!I->hasFnAttr(Attribute::StrictFP))
   I->addAttribute(AttributeList::FunctionIndex, Attribute::StrictFP);
-setConstrainedFPFunctionAttr();
   }
 
   //======//
Index: clang/test/Driver/fp-model.c
===
--- /dev/null
+++ clang/test/Driver/fp-model.c
@@ -0,0 +1,137 @@
+// Test that incompatible combinations of -ffp-model= options
+// and other floating point options get a warning diagnostic.
+//
+// REQUIRES: clang-driver
+
+// RUN: %clang -### -ffp-model=fast -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN %s
+// WARN: warning: overriding '-ffp-model=fast' option with '-ffp-contract=off' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=fast -ffp-contract=on -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN1 %s
+// WARN1: warning: overriding '-ffp-model=fast' option with '-ffp-contract=on' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fassociative-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN2 %s
+// WARN2: warning: overriding '-ffp-model=strict' option with '-fassociative-math' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN3 %s
+// WARN3: warning: overriding '-ffp-model=strict' option with '-ffast-math' [-Woverriding-t-option]
+
+// RUN: %clang -### 

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2337
+<< "-ffp-contract=fast";
+optID = options::OPT_ffast_math;
+FPModel = Val;

Here it seems you are changing `optID` to `OPT_ffast_math` to reuse the logic 
specified below for that case to reset the state of the floating point options.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2345
+<< "-ffp-contract=fast";
+optID = options::OPT_ffp_contract;
+FPModel = Val;

Here the state of the floating point options seems unchanged except for 
`FPContract`. If I run `clang -ffp-model=fast -ffp-model=precise`, I would 
expect the state of the floating point options to match the one of 
`-fno-fast-math` except for `FPContract` which you want to be set to "fast".

I think you might need to replicate the reset for all the option here as well, 
so at this point I don't know how much worth is to use the optID reset trick 
for the "fast" case only.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2576
+CmdArgs.push_back("-ftrapping-math");
+  } else if (TrappingMathPresent)
 CmdArgs.push_back("-fno-trapping-math");

mibintc wrote:
> michele.scandale wrote:
> > With this change if I run `clang -### -ffast-math test.c` I don't see 
> > `-fno-trapping-math` passed to the CC1. 
> > This is changing the changes the value of the function level attribute 
> > "no-trapping-math" (see lib/CodeGen/CGCall.cpp : 1747).
> > Is this an intended change?
> > 
> > Moreover since with this patch the default value for trapping math changed, 
> > the "no-trapping-math" function level attribute is incorrect also for 
> > default case.
> Before this patch, ftrapping-math was added to the Driver and also a 
> bitfield, ``NoTrappingFPMath`` was created in the LLVM to describe the state 
> of trapping-math, but otherwise that bit wasn't consulted and the option had 
> no effect.  Gcc describes ftrapping-math as the default, but in llvm by 
> default floating point exceptions are masked and this corresponds to the 
> floating point Constrained Intrinsics having exception behavior set to 
> ignored.  This patch changed the llvm constructor to set the trapping bit to 
> "no trap".  In fact I'd like to get rid of the ``NoTrappingFPMath`` bitfield 
> since it's not being used, but I didn't make that change at this point. 
> 
> If I remember correctly, there are a bunch of driver tests that failed if 
> fno-trapping-math is output to cc1. I'd have to reconstruct the details.  
> Since fno-trapping-math is the default, it isn't passed through on the cc1 
> command line: the Clang.cpp driver doesn't pass through the positive and 
> negative for each existing option.
> 
> Thanks for pointing out the line in CGCall.cpp, it seems the CodeGenOpts 
> aren't getting set up perfectly I'll fix that in CompilerInvocation.cpp; I 
> don't see anything setting trapping-math as part of function level attribute, 
> @michele.scandale  did I overlook that/can you point out where that is?
I guess you are referring to the code in `TargetMachine.cpp` where the function 
level attributes are used to reset the `TargetOptions` state whenever we 
initiate the backend codegen for a given function. Considering that the 
trapping math option as stated in the documentation did not have any effect, 
I'm not surprised to see not many uses. The only one I can see is in 
`llvm/lib/Target/ARM/ARMAsmPrinter.cpp : 687` where the function level 
attribute affects the emission of some ARM specific attributes.

My only concern was that the change of the default value for trapping math was 
not propagated entirely causing this function level attribute to be initialized 
incorrectly.
Fixing the logic in `CompilerInvocation.cpp` considering the change of default 
it is fine for me.

Given that `ffp-exception-behavior={ignore,maytrap,strict}` supersedes 
`-f{,no-}trapping-math` I would expect long term to see the internal state of 
the compiler frontend to only care about the new state `FPExceptionBehavior` 
for both language and code generation options. And I guess the same would apply 
to the backend stage as well.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

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

Here's an update in response to comments from @michele.scandale 
I fixed the assertion error and added a test case
I fixed the setting of ftrapping-math in CodeGenOpts
I deleted an incorrect comment
I added a diagnostic when -fp-exception-behavior= is overridden on the command 
line by f[no-]trapping-math
I updated one of the test cases to work with some small modifications that will 
be made to IRBuilder.h


Repository:
  rG LLVM Github Monorepo

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.def
  clang/include/clang/Basic/LangOptions.h
  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/CodeGen/fpconstrained.cpp
  clang/test/Driver/clang_f_opts.c
  clang/test/Driver/fast-math.c
  clang/test/Driver/fp-model.c
  llvm/include/llvm/IR/IRBuilder.h
  llvm/include/llvm/Target/TargetOptions.h

Index: llvm/include/llvm/Target/TargetOptions.h
===
--- llvm/include/llvm/Target/TargetOptions.h
+++ llvm/include/llvm/Target/TargetOptions.h
@@ -107,7 +107,7 @@
   public:
 TargetOptions()
 : PrintMachineCode(false), UnsafeFPMath(false), NoInfsFPMath(false),
-  NoNaNsFPMath(false), NoTrappingFPMath(false),
+  NoNaNsFPMath(false), NoTrappingFPMath(true),
   NoSignedZerosFPMath(false),
   HonorSignDependentRoundingFPMathOption(false), NoZerosInBSS(false),
   GuaranteedTailCallOpt(false), StackSymbolOrdering(true),
Index: llvm/include/llvm/IR/IRBuilder.h
===
--- llvm/include/llvm/IR/IRBuilder.h
+++ llvm/include/llvm/IR/IRBuilder.h
@@ -259,7 +259,7 @@
 assert(BB && "Must have a basic block to set any function attributes!");
 
 Function *F = BB->getParent();
-if (!F->hasFnAttribute(Attribute::StrictFP)) {
+if (F && !F->hasFnAttribute(Attribute::StrictFP)) {
   F->addFnAttr(Attribute::StrictFP);
 }
   }
Index: clang/test/Driver/fp-model.c
===
--- /dev/null
+++ clang/test/Driver/fp-model.c
@@ -0,0 +1,137 @@
+// Test that incompatible combinations of -ffp-model= options
+// and other floating point options get a warning diagnostic.
+//
+// REQUIRES: clang-driver
+
+// RUN: %clang -### -ffp-model=fast -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN %s
+// WARN: warning: overriding '-ffp-model=fast' option with '-ffp-contract=off' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=fast -ffp-contract=on -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN1 %s
+// WARN1: warning: overriding '-ffp-model=fast' option with '-ffp-contract=on' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fassociative-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN2 %s
+// WARN2: warning: overriding '-ffp-model=strict' option with '-fassociative-math' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN3 %s
+// WARN3: warning: overriding '-ffp-model=strict' option with '-ffast-math' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffinite-math-only -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN4 %s
+// WARN4: warning: overriding '-ffp-model=strict' option with '-ffinite-math-only' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffp-contract=fast -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN5 %s
+// WARN5: warning: overriding '-ffp-model=strict' option with '-ffp-contract=fast' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN6 %s
+// WARN6: warning: overriding '-ffp-model=strict' option with '-ffp-contract=off' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffp-contract=on -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN7 %s
+// WARN7: warning: overriding '-ffp-model=strict' option with '-ffp-contract=on' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fno-honor-infinities -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN8 %s
+// WARN8: warning: overriding '-ffp-model=strict' option with '-fno-honor-infinities' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fno-honor-nans -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN9 %s
+// WARN9: warning: overriding '-ffp-model=strict' option with '-fno-honor-nans' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fno-rounding-math -c %s 2>&1 \
+// RUN:   | FileCheck 

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added inline comments.



Comment at: llvm/include/llvm/IR/IRBuilder.h:262
 Function *F = BB->getParent();
-if (!F->hasFnAttribute(Attribute::StrictFP)) {
+if (F && !F->hasFnAttribute(Attribute::StrictFP)) {
   F->addFnAttr(Attribute::StrictFP);

kpn wrote:
> rjmccall wrote:
> > kpn wrote:
> > > This looks reasonable to me. 
> > > 
> > > It smells like there's a larger strictfp IRBuilder issue, but that's not 
> > > an issue for this patch here. The larger issue won't be hit since the new 
> > > options affect the entire compilation. It therefore shouldn't block this 
> > > patch.
> > Does IRBuilder actually support inserting into an unparented basic block?  
> > I feel like this is exposing a much more serious mis-use of IRBuilder.
> I suspect you are correct. If we let this "F && " change go in we'll have a 
> situation where whether or not a block is currently in a function when a 
> function call is emitted will affect whether or not the eventual function 
> definition gets the strictfp attribute. That seems like an unfortunate 
> inconsistency.
> 
> I'm still looking into it. I hope to have an IRBuilder review up today or 
> tomorrow.
As I just commented on the related patch @kpn posted, it appears that IRBuilder 
doesn't entirely support inserting into an unparented block. I was surprised by 
this, but there are places that need to be able to get to the Module from the 
BasicBlock. So, I think something problematic may be happening in the failing 
case.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

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

inline replies to Michele, will upload a new patch shortly




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2408
+case options::OPT_frounding_math:
+  // The default setting for frounding-math is True and ffast-math
+  // sets fno-rounding-math, but we only want to use constrained

michele.scandale wrote:
> Isn't the default `-fno-rounding-math`?
Yes the default is no rounding math, I'll remove the comment.  Thank you. 



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2416
+  RoundingFPMath = false;
+  RoundingMathPresent = false;
+  break;

michele.scandale wrote:
> Shouldn't this be set to `true` similarly to what you do for 
> `TrappingMathPresent ` to track whether there is an explicit option related 
> to rounding math?
There's a switch statement above this that interprets the command line option 
-fp-model=strict as though frounding had appeared on the command line by 
assigning a new value to optID so that's why there is a discrepancy.  Also I'm 
using the *Present boolean variables to preserve the output from Driver so that 
pre-existing driver test cases don't need to be changed.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2574
+// FP Exception Behavior is also set to strict
+assert(FPExceptionBehavior.equals("strict"));
+CmdArgs.push_back("-ftrapping-math");

michele.scandale wrote:
> Running `clang -### -ftrapping-math -ffp-exception-behavior=ignore` lead to 
> this assertion to fail. As far as I can see `TrappingMath` is not changed in 
> the case FPExceptionBehavior is "ignore" or "maytrap".
> Clearly in the "ignore" case it should be safe to just set `TrappingMath` to 
> false, but I'm not sure about the "maytrap" case.
> It seems that `-ffp-exception-behavior` is more general than 
> `-f{,no-}trapping-math`, so it seems natural to me to see `ftrapping-math` 
> and `foo-trapping-math` as aliases for `ffp-exception-behavior=strict` and 
> `ffp-exception-behavior=ignore` respectively. If we agree on this, then I 
> would expect the reasoning inside the compiler only in terms of 
> `FPExceptionBehavior`.
Thanks for pointing out this assertion failure, I will upload a patch with fix. 
 Yes we could entirely express ftrapping-math and fno-trapping-math via the 
ffp-exception-behavior= option. That would probably be better--currently the 
trapping option becomes effective via the exception behavior parameter to the 
llvm floating point constrained intrinsics, and it can take 3 values. I thought 
it would be too radical at the moment, so I didn't propose that in this patch.

In the patch I'm about to add, I added a test case for the assertion that you 
saw.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2576
+CmdArgs.push_back("-ftrapping-math");
+  } else if (TrappingMathPresent)
 CmdArgs.push_back("-fno-trapping-math");

michele.scandale wrote:
> With this change if I run `clang -### -ffast-math test.c` I don't see 
> `-fno-trapping-math` passed to the CC1. 
> This is changing the changes the value of the function level attribute 
> "no-trapping-math" (see lib/CodeGen/CGCall.cpp : 1747).
> Is this an intended change?
> 
> Moreover since with this patch the default value for trapping math changed, 
> the "no-trapping-math" function level attribute is incorrect also for default 
> case.
Before this patch, ftrapping-math was added to the Driver and also a bitfield, 
``NoTrappingFPMath`` was created in the LLVM to describe the state of 
trapping-math, but otherwise that bit wasn't consulted and the option had no 
effect.  Gcc describes ftrapping-math as the default, but in llvm by default 
floating point exceptions are masked and this corresponds to the floating point 
Constrained Intrinsics having exception behavior set to ignored.  This patch 
changed the llvm constructor to set the trapping bit to "no trap".  In fact I'd 
like to get rid of the ``NoTrappingFPMath`` bitfield since it's not being used, 
but I didn't make that change at this point. 

If I remember correctly, there are a bunch of driver tests that failed if 
fno-trapping-math is output to cc1. I'd have to reconstruct the details.  Since 
fno-trapping-math is the default, it isn't passed through on the cc1 command 
line: the Clang.cpp driver doesn't pass through the positive and negative for 
each existing option.

Thanks for pointing out the line in CGCall.cpp, it seems the CodeGenOpts aren't 
getting set up perfectly I'll fix that in CompilerInvocation.cpp; I don't see 
anything setting trapping-math as part of function level attribute, 
@michele.scandale  did I overlook that/can you point out where that is?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

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

kpn added inline comments.



Comment at: llvm/include/llvm/IR/IRBuilder.h:262
 Function *F = BB->getParent();
-if (!F->hasFnAttribute(Attribute::StrictFP)) {
+if (F && !F->hasFnAttribute(Attribute::StrictFP)) {
   F->addFnAttr(Attribute::StrictFP);

rjmccall wrote:
> kpn wrote:
> > This looks reasonable to me. 
> > 
> > It smells like there's a larger strictfp IRBuilder issue, but that's not an 
> > issue for this patch here. The larger issue won't be hit since the new 
> > options affect the entire compilation. It therefore shouldn't block this 
> > patch.
> Does IRBuilder actually support inserting into an unparented basic block?  I 
> feel like this is exposing a much more serious mis-use of IRBuilder.
I suspect you are correct. If we let this "F && " change go in we'll have a 
situation where whether or not a block is currently in a function when a 
function call is emitted will affect whether or not the eventual function 
definition gets the strictfp attribute. That seems like an unfortunate 
inconsistency.

I'm still looking into it. I hope to have an IRBuilder review up today or 
tomorrow.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2408
+case options::OPT_frounding_math:
+  // The default setting for frounding-math is True and ffast-math
+  // sets fno-rounding-math, but we only want to use constrained

Isn't the default `-fno-rounding-math`?



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2416
+  RoundingFPMath = false;
+  RoundingMathPresent = false;
+  break;

Shouldn't this be set to `true` similarly to what you do for 
`TrappingMathPresent ` to track whether there is an explicit option related to 
rounding math?



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2574
+// FP Exception Behavior is also set to strict
+assert(FPExceptionBehavior.equals("strict"));
+CmdArgs.push_back("-ftrapping-math");

Running `clang -### -ftrapping-math -ffp-exception-behavior=ignore` lead to 
this assertion to fail. As far as I can see `TrappingMath` is not changed in 
the case FPExceptionBehavior is "ignore" or "maytrap".
Clearly in the "ignore" case it should be safe to just set `TrappingMath` to 
false, but I'm not sure about the "maytrap" case.
It seems that `-ffp-exception-behavior` is more general than 
`-f{,no-}trapping-math`, so it seems natural to me to see `ftrapping-math` and 
`foo-trapping-math` as aliases for `ffp-exception-behavior=strict` and 
`ffp-exception-behavior=ignore` respectively. If we agree on this, then I would 
expect the reasoning inside the compiler only in terms of `FPExceptionBehavior`.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2576
+CmdArgs.push_back("-ftrapping-math");
+  } else if (TrappingMathPresent)
 CmdArgs.push_back("-fno-trapping-math");

With this change if I run `clang -### -ffast-math test.c` I don't see 
`-fno-trapping-math` passed to the CC1. 
This is changing the changes the value of the function level attribute 
"no-trapping-math" (see lib/CodeGen/CGCall.cpp : 1747).
Is this an intended change?

Moreover since with this patch the default value for trapping math changed, the 
"no-trapping-math" function level attribute is incorrect also for default case.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

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

kpn added a comment.

In D62731#1750427 , @mibintc wrote:

> In D62731#1750412 , @kpn wrote:
>
> > Does anyone think a warning is appropriate because the new flags are 
> > exercising experimental, incomplete code in both clang and llvm? The 
> > warning would be removed when we believe the feature is complete and ready 
> > to use.
>
>
> @kpn Can you say more about "incomplete code in ... clang".  I don't know 
> what's missing from clang.


See D70256 . Calls to clang's math builtins 
that are llvm intrinsics need to be changed to be calls to the constrained 
intrinsics. I've been waiting to submit the patches because they weren't 
testable without these command line options.

There may be other issues as well. I'm not sure.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: llvm/include/llvm/IR/IRBuilder.h:262
 Function *F = BB->getParent();
-if (!F->hasFnAttribute(Attribute::StrictFP)) {
+if (F && !F->hasFnAttribute(Attribute::StrictFP)) {
   F->addFnAttr(Attribute::StrictFP);

kpn wrote:
> This looks reasonable to me. 
> 
> It smells like there's a larger strictfp IRBuilder issue, but that's not an 
> issue for this patch here. The larger issue won't be hit since the new 
> options affect the entire compilation. It therefore shouldn't block this 
> patch.
Does IRBuilder actually support inserting into an unparented basic block?  I 
feel like this is exposing a much more serious mis-use of IRBuilder.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

In D62731#1750412 , @kpn wrote:

> Does anyone think a warning is appropriate because the new flags are 
> exercising experimental, incomplete code in both clang and llvm? The warning 
> would be removed when we believe the feature is complete and ready to use.


@kpn Can you say more about "incomplete code in ... clang".  I don't know 
what's missing from clang.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

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

kpn added a comment.

Does anyone think a warning is appropriate because the new flags are exercising 
experimental, incomplete code in both clang and llvm? The warning would be 
removed when we believe the feature is complete and ready to use.




Comment at: llvm/include/llvm/IR/IRBuilder.h:262
 Function *F = BB->getParent();
-if (!F->hasFnAttribute(Attribute::StrictFP)) {
+if (F && !F->hasFnAttribute(Attribute::StrictFP)) {
   F->addFnAttr(Attribute::StrictFP);

This looks reasonable to me. 

It smells like there's a larger strictfp IRBuilder issue, but that's not an 
issue for this patch here. The larger issue won't be hit since the new options 
affect the entire compilation. It therefore shouldn't block this patch.


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

I added a nullptr check in IRBuilder.h and a test case to cover the segfault 
reported in https://bugs.llvm.org/show_bug.cgi?id=44048


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc updated this revision to Diff 229893.
mibintc added a reviewer: kpn.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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.def
  clang/include/clang/Basic/LangOptions.h
  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/CodeGen/fpconstrained.cpp
  clang/test/Driver/clang_f_opts.c
  clang/test/Driver/fast-math.c
  clang/test/Driver/fp-model.c
  llvm/include/llvm/IR/IRBuilder.h
  llvm/include/llvm/Target/TargetOptions.h

Index: llvm/include/llvm/Target/TargetOptions.h
===
--- llvm/include/llvm/Target/TargetOptions.h
+++ llvm/include/llvm/Target/TargetOptions.h
@@ -107,7 +107,7 @@
   public:
 TargetOptions()
 : PrintMachineCode(false), UnsafeFPMath(false), NoInfsFPMath(false),
-  NoNaNsFPMath(false), NoTrappingFPMath(false),
+  NoNaNsFPMath(false), NoTrappingFPMath(true),
   NoSignedZerosFPMath(false),
   HonorSignDependentRoundingFPMathOption(false), NoZerosInBSS(false),
   GuaranteedTailCallOpt(false), StackSymbolOrdering(true),
Index: llvm/include/llvm/IR/IRBuilder.h
===
--- llvm/include/llvm/IR/IRBuilder.h
+++ llvm/include/llvm/IR/IRBuilder.h
@@ -259,7 +259,7 @@
 assert(BB && "Must have a basic block to set any function attributes!");
 
 Function *F = BB->getParent();
-if (!F->hasFnAttribute(Attribute::StrictFP)) {
+if (F && !F->hasFnAttribute(Attribute::StrictFP)) {
   F->addFnAttr(Attribute::StrictFP);
 }
   }
Index: clang/test/Driver/fp-model.c
===
--- /dev/null
+++ clang/test/Driver/fp-model.c
@@ -0,0 +1,130 @@
+// Test that incompatible combinations of -ffp-model= options
+// and other floating point options get a warning diagnostic.
+//
+// REQUIRES: clang-driver
+
+// RUN: %clang -### -ffp-model=fast -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN %s
+// WARN: warning: overriding '-ffp-model=fast' option with '-ffp-contract=off' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=fast -ffp-contract=on -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN1 %s
+// WARN1: warning: overriding '-ffp-model=fast' option with '-ffp-contract=on' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fassociative-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN2 %s
+// WARN2: warning: overriding '-ffp-model=strict' option with '-fassociative-math' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN3 %s
+// WARN3: warning: overriding '-ffp-model=strict' option with '-ffast-math' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffinite-math-only -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN4 %s
+// WARN4: warning: overriding '-ffp-model=strict' option with '-ffinite-math-only' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffp-contract=fast -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN5 %s
+// WARN5: warning: overriding '-ffp-model=strict' option with '-ffp-contract=fast' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN6 %s
+// WARN6: warning: overriding '-ffp-model=strict' option with '-ffp-contract=off' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffp-contract=on -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN7 %s
+// WARN7: warning: overriding '-ffp-model=strict' option with '-ffp-contract=on' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fno-honor-infinities -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN8 %s
+// WARN8: warning: overriding '-ffp-model=strict' option with '-fno-honor-infinities' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fno-honor-nans -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN9 %s
+// WARN9: warning: overriding '-ffp-model=strict' option with '-fno-honor-nans' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fno-rounding-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARNa %s
+// WARNa: warning: overriding '-ffp-model=strict' option with '-fno-rounding-math' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fno-signed-zeros -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARNb %s
+// WARNb: warning: overriding '-ffp-model=strict' option with '-fno-signed-zeros' [-Woverriding-t-option]
+
+// RUN: %clang -### 

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

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

Reopening since patch was reverted


Repository:
  rG LLVM Github Monorepo

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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Eric Christopher via cfe-commits]

No, it's just the bug that Jorge found. I understand it might have
highlighted an existing bug, but reverting back to clean is the best
bet here and then we can recommit when we get the latent bug fixed :)

On Mon, Nov 18, 2019 at 11:08 AM Melanie Blower via Phabricator
 wrote:
>
> mibintc added a comment.
>
> In D62731#1750312 , @echristo wrote:
>
> > In D62731#1749916 , @mibintc wrote:
> >
> > > Thanks I see it, I'm working on a patch. Previously there was no support 
> > > for frounding-math (unimplemented). This patch enables the option. In the 
> > > IR builder, there's a call to a runtime function in the exception handler 
> > > which is unexpectedly null.  I start by adding a null pointer check.
> >
> >
> > Had a crash on valid here for days, let's revert and then get a fix when 
> > recommitting. I'll respond to the thread when reverting. Thanks :)
>
>
> @echristo I just saw the bug was reported today, is the "crash on valid" 
> visible on the bots?  Can you provide url showing same? Thanks
> I opened https://bugs.llvm.org/show_bug.cgi?id=44048 for @jgorbe
>
>
> 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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

In D62731#1750312 , @echristo wrote:

> In D62731#1749916 , @mibintc wrote:
>
> > Thanks I see it, I'm working on a patch. Previously there was no support 
> > for frounding-math (unimplemented). This patch enables the option. In the 
> > IR builder, there's a call to a runtime function in the exception handler 
> > which is unexpectedly null.  I start by adding a null pointer check.
>
>
> Had a crash on valid here for days, let's revert and then get a fix when 
> recommitting. I'll respond to the thread when reverting. Thanks :)


@echristo I just saw the bug was reported today, is the "crash on valid" 
visible on the bots?  Can you provide url showing same? Thanks
I opened https://bugs.llvm.org/show_bug.cgi?id=44048 for @jgorbe


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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

The incorrect code is actually in the IRBuilder which is part of a different 
patch...

In D62731#1750312 , @echristo wrote:

> In D62731#1749916 , @mibintc wrote:
>
> > Thanks I see it, I'm working on a patch. Previously there was no support 
> > for frounding-math (unimplemented). This patch enables the option. In the 
> > IR builder, there's a call to a runtime function in the exception handler 
> > which is unexpectedly null.  I start by adding a null pointer check.
>
>
> Had a crash on valid here for days, let's revert and then get a fix when 
> recommitting. I'll respond to the thread when reverting. 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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Eric Christopher via Phabricator via cfe-commits]

echristo added a comment.

In D62731#1749916 , @mibintc wrote:

> Thanks I see it, I'm working on a patch. Previously there was no support for 
> frounding-math (unimplemented). This patch enables the option. In the IR 
> builder, there's a call to a runtime function in the exception handler which 
> is unexpectedly null.  I start by adding a null pointer check.


Had a crash on valid here for days, let's revert and then get a fix when 
recommitting. I'll respond to the thread when reverting. 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

Re: [PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Eric Christopher via cfe-commits]

Hi All,

I've gone ahead and temporarily reverted here:

echristo@jhereg ~/s/llvm-project> git push
To github.com:llvm/llvm-project.git
   a77b66a0562..30e7ee3c4ba  master -> master

and we can just reapply when the issues are fixed. Thanks :)

-eric

On Mon, Nov 18, 2019 at 6:58 AM Melanie Blower via Phabricator via
cfe-commits  wrote:
>
> mibintc added a comment.
>
> Thanks I see it, I'm working on a patch. Previously there was no support for 
> frounding-math (unimplemented). This patch enables the option. In the IR 
> builder, there's a call to a runtime function in the exception handler which 
> is unexpectedly null.  I start by adding a null pointer check.
>
>
> 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

Thanks I see it, I'm working on a patch. Previously there was no support for 
frounding-math (unimplemented). This patch enables the option. In the IR 
builder, there's a call to a runtime function in the exception handler which is 
unexpectedly null.  I start by adding a null pointer check.


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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jorge Gorbe Moya via Phabricator via cfe-commits]

jgorbe added a comment.

Hi,

I found a clang crash that seems to be caused by this patch. Here's a reduced 
test case:

  template 
  class a {
   public:
~a();
void b();
  };
  
  template 
  a::~a() try {
b();
  } catch (...) {
  }
  
  class d {
   public:
d(const char *, int);
a e;
  }
  
  d("", 1);

Building it with `clang -c -frounding-math` results in the following crash:

  Stack dump:
  0.  Program arguments: 
/usr/local/google/home/jgorbe/code/llvm-build/bin/clang-10 -cc1 -triple 
x86_64-unknown-linux-gnu -emit-obj -mrelax-all -disable-free -main-fi
  le-name repro.cc -mrelocation-model static -mthread-model posix 
-mframe-pointer=all -fmath-errno -frounding-math -masm-verbose 
-mconstructor-aliases -munwind-tables -fu
  se-init-array -target-cpu x86-64 -dwarf-column-info -debugger-tuning=gdb 
-resource-dir /usr/local/google/home/jgorbe/code/llvm-build/lib/clang/10.0.0 
-internal-isystem 
  /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8 -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/x86_64-linux-gnu/c++/8 
-internal-isystem
   /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/x86_64-linux-gnu/c++/8 
-internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/backward -intern
  al-isystem /usr/local/include -internal-isystem 
/usr/local/google/home/jgorbe/code/llvm-build/lib/clang/10.0.0/include 
-internal-externc-isystem /usr/include/x86_64-lin
  ux-gnu -internal-externc-isystem /include -internal-externc-isystem 
/usr/include -fdeprecated-macro -fdebug-compilation-dir 
/usr/local/google/home/jgorbe/repro4 -ferror
  -limit 19 -fmessage-length 0 -fgnuc-version=4.2.1 -fobjc-runtime=gcc 
-fcxx-exceptions -fexceptions -fdiagnostics-show-option -fcolor-diagnostics 
-faddrsig -o /tmp/repro
  -c9cae0.o -x c++ repro.cc 
  1.   parser at end of file
  2.  Per-file LLVM IR generation
  3.  repro.cc:4:3: Generating code for declaration 'a::~a'
   #0 0x07fb2b27 llvm::sys::PrintStackTrace(llvm::raw_ostream&) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Unix/Signals.inc:532:11
 
   #1 0x07fb2cc9 PrintStackTraceSignalHandler(void*) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Unix/Signals.inc:593:1 

   #2 0x07fb160b llvm::sys::RunSignalHandlers() 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Signals.cpp:67:5   
 
   #3 0x07fb3415 SignalHandler(int) 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Unix/Signals.inc:384:1 
   
   #4 0x7f6789ec03a0 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x123a0) 

   #5 0x0739af45 
llvm::AttributeList::hasFnAttribute(llvm::Attribute::AttrKind) const 
/usr/local/google/home/jgorbe/code/llvm/llvm/lib/IR/Attributes.cpp:1315:10
   #6 0x051a1964 
llvm::Function::hasFnAttribute(llvm::Attribute::AttrKind) const 
/usr/local/google/home/jgorbe/code/llvm/llvm/include/llvm/IR/Function.h:324:5   
 
   #7 0x052421a1 llvm::IRBuilderBase::setConstrainedFPFunctionAttr() 
/usr/local/google/home/jgorbe/code/llvm/llvm/include/llvm/IR/IRBuilder.h:262:9
   #8 0x05241b60 
llvm::IRBuilderBase::setConstrainedFPCallAttr(llvm::CallInst*) 
/usr/local/google/home/jgorbe/code/llvm/llvm/include/llvm/IR/IRBuilder.h:271:3
   #9 0x084a516b llvm::IRBuilder::CreateCall(llvm::FunctionType*, 
llvm::Value*, llvm::ArrayRef, 
llvm::ArrayRef >, llvm::Twine const&, 
llvm::MDNode*) 
/usr/local/google/home/jgorbe/code/llvm/llvm/include/llvm/IR/IRBuilder.h:2274:9 


  #10 0x084a4488 llvm::IRBuilder::CreateCall(llvm::FunctionCallee, 
llvm::ArrayRef, 
llvm::ArrayRef >, llvm::Twine const&, 
llvm::MDNode*) 
/usr/local/google/home/jgorbe/code/llvm/llvm/include/llvm/IR/IRBuilder.h:2288:5
  #11 0x0849288c 
clang::CodeGen::CodeGenFunction::EmitRuntimeCall(llvm::FunctionCallee, 
llvm::ArrayRef, llvm::Twine const&) 
/usr/local/google/home/jgorbe/code/llvm/clang/lib/CodeGen/CGCall.cpp:3726:26
  #12 0x0849278d 
clang::CodeGen::CodeGenFunction::EmitNounwindRuntimeCall(llvm::FunctionCallee, 
llvm::ArrayRef, llvm::Twine const&) 
/usr/local/google/home/jgorbe/code/llvm/clang/lib/CodeGen/CGCall.cpp:3691:19


  #13 0x0897b3e4 (anonymous 
namespace)::ItaniumCXXABI::emitTerminateForUnexpectedException(clang::CodeGen::CodeGenFunction&,
 llvm::Value*) 
/usr/local/google/home/jgorbe/code/llvm/clang/lib/CodeGen/ItaniumCXXABI.cpp:4364:5
   
  #14 0x0865698d clang::CodeGen::CodeGenFunction::getTerminateHandler() 

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc closed this revision.
mibintc added a comment.

Don't know why the commit id didn't get linked when I pushed the change. Here's 
the closure info:

commit af57dbf12e54f3a8ff48534bf1078f4de104c1cd 

Author: Melanie Blower 
Date:   Tue Nov 5 13:41:21 2019 -0800

  Add support for options -frounding-math, ftrapping-math, -ffp-model=, and 
-ffp-exception-behavior=


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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Looks okay to me.


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: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

mibintc updated this revision to Diff 227960.
mibintc retitled this revision from "Add support for options -frounding-math, 
ftrapping-math, -fp-model=, and -fp-exception-behavior=, : Specify floating 
point behavior" to "Add support for options -frounding-math, -ftrapping-math, 
-ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior".
mibintc added a comment.

When doing final testing before commit, I used the Debug build with assertions 
enabled and found that a couple tests failed with assertion in Clang.cpp. I 
made some modifications there so the assert wouldn't be triggered. Also I fixed 
some spelling errors in the comments. (option name misspelled: missing the 
first f)

when "make check-all" with Debug build, I see lit test failure 
llvm/test/Object/macho-invalid.test; I think that fail is not related to my 
change.

@rjmccall thanks for all your help developing this patch


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.def
  clang/include/clang/Basic/LangOptions.h
  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
  clang/test/Driver/fast-math.c
  clang/test/Driver/fp-model.c
  llvm/include/llvm/Target/TargetOptions.h

Index: llvm/include/llvm/Target/TargetOptions.h
===
--- llvm/include/llvm/Target/TargetOptions.h
+++ llvm/include/llvm/Target/TargetOptions.h
@@ -107,7 +107,7 @@
   public:
 TargetOptions()
 : PrintMachineCode(false), UnsafeFPMath(false), NoInfsFPMath(false),
-  NoNaNsFPMath(false), NoTrappingFPMath(false),
+  NoNaNsFPMath(false), NoTrappingFPMath(true),
   NoSignedZerosFPMath(false),
   HonorSignDependentRoundingFPMathOption(false), NoZerosInBSS(false),
   GuaranteedTailCallOpt(false), StackSymbolOrdering(true),
Index: clang/test/Driver/fp-model.c
===
--- /dev/null
+++ clang/test/Driver/fp-model.c
@@ -0,0 +1,130 @@
+// Test that incompatible combinations of -ffp-model= options
+// and other floating point options get a warning diagnostic.
+//
+// REQUIRES: clang-driver
+
+// RUN: %clang -### -ffp-model=fast -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN %s
+// WARN: warning: overriding '-ffp-model=fast' option with '-ffp-contract=off' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=fast -ffp-contract=on -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN1 %s
+// WARN1: warning: overriding '-ffp-model=fast' option with '-ffp-contract=on' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fassociative-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN2 %s
+// WARN2: warning: overriding '-ffp-model=strict' option with '-fassociative-math' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffast-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN3 %s
+// WARN3: warning: overriding '-ffp-model=strict' option with '-ffast-math' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffinite-math-only -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN4 %s
+// WARN4: warning: overriding '-ffp-model=strict' option with '-ffinite-math-only' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffp-contract=fast -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN5 %s
+// WARN5: warning: overriding '-ffp-model=strict' option with '-ffp-contract=fast' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffp-contract=off -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN6 %s
+// WARN6: warning: overriding '-ffp-model=strict' option with '-ffp-contract=off' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -ffp-contract=on -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN7 %s
+// WARN7: warning: overriding '-ffp-model=strict' option with '-ffp-contract=on' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fno-honor-infinities -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN8 %s
+// WARN8: warning: overriding '-ffp-model=strict' option with '-fno-honor-infinities' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fno-honor-nans -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARN9 %s
+// WARN9: warning: overriding '-ffp-model=strict' option with '-fno-honor-nans' [-Woverriding-t-option]
+
+// RUN: %clang -### -ffp-model=strict -fno-rounding-math -c %s 2>&1 \
+// RUN:   | FileCheck --check-prefix=WARNa %s
+// WARNa: warning: overriding '-ffp-model=strict' option with '-fno-rounding-math' [-Woverriding-t-option]
+
+// RUN: %clang