[PATCH] D104854: Introduce intrinsic llvm.isnan

[Thomas Preud'homme via Phabricator via cfe-commits]

thopre added a comment.

Ah fair enough. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

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

kpn added a comment.

In D104854#2959680 , @thopre wrote:

> In D104854#2957735 , @kpn wrote:
>
>> In D104854#2957490 , @lebedev.ri 
>> wrote:
>>
>>> In D104854#2957471 , @sepavloff 
>>> wrote:
>>>
 In D104854#2957423 , @spatel 
 wrote:

> Is it intentional that we are not canonicalizing the intrinsic call back 
> to `fcmp uno` in the default FP environment?

 It is lowered to unordered comparison by default. Changing `llvm.isnan` to 
  `fcmp uno` somewhere in IR would make it possible to optimize out the 
 latter if fast-math mode is on. Preserving semantics of `isnan` when 
 fast-math is in effect was one of the goals of this change.
>>>
>>> Eeek. Was there an RFC about this?
>>> This does not sound good to me at all,
>>> much like "let's not apply fast-math flags to x86 vector intrinsics".
>>
>> We can switch into and out of the default FP environment inside a single 
>> function.
>
> Really? The constrained intrinsic documentation claims the reverse 
> (https://llvm.org/docs/LangRef.html#constrainedfp):
>
>> If any FP operation in a function is constrained then they all must be 
>> constrained. This is required for correct LLVM IR. Optimizations that move 
>> code around can create miscompiles if mixing of constrained and normal 
>> operations is done. The correct way to mix constrained and less constrained 
>> operations is to use the rounding mode and exception handling metadata to 
>> mark constrained intrinsics as having LLVM’s default behavior.

Use of constrained intrinsics does not mean that we are automatically in an 
alternate FP environment.

When constrained intrinsics are used and the metadata says the rounding mode is 
"tonearest" with exceptions set to "ignore" then that's the default FP 
environment. If, for example, #pragma STDC FENV_ACCESS is used in only a part 
of a function then the constrained intrinsics will be used in the entire 
function but the metadata will specify different exception or rounding behavior 
in the part covered by the FENV_ACCESS.

That the constrained intrinsics can state that they are in the default FP 
environment is what makes it safe for EarlyCSE to treat them the same as a 
normal FP instruction (which is assumed to be in the default FP environment). 
For example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

I posted the RFC: 
https://lists.llvm.org/pipermail/llvm-dev/2021-August/152257.html

Depending on the feedback I'll revert the check or modify the implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a subscriber: hans.
lebedev.ri added a comment.

In D104854#2959680 , @thopre wrote:

> In D104854#2957735 , @kpn wrote:
>
>> In D104854#2957490 , @lebedev.ri 
>> wrote:
>>
>>> In D104854#2957471 , @sepavloff 
>>> wrote:
>>>
 In D104854#2957423 , @spatel 
 wrote:

> Is it intentional that we are not canonicalizing the intrinsic call back 
> to `fcmp uno` in the default FP environment?

 It is lowered to unordered comparison by default. Changing `llvm.isnan` to 
  `fcmp uno` somewhere in IR would make it possible to optimize out the 
 latter if fast-math mode is on. Preserving semantics of `isnan` when 
 fast-math is in effect was one of the goals of this change.
>>>
>>> Eeek. Was there an RFC about this?
>>> This does not sound good to me at all,
>>> much like "let's not apply fast-math flags to x86 vector intrinsics".
>>
>> We can switch into and out of the default FP environment inside a single 
>> function.
>
> Really? The constrained intrinsic documentation claims the reverse 
> (https://llvm.org/docs/LangRef.html#constrainedfp):
>
>> If any FP operation in a function is constrained then they all must be 
>> constrained. This is required for correct LLVM IR. Optimizations that move 
>> code around can create miscompiles if mixing of constrained and normal 
>> operations is done. The correct way to mix constrained and less constrained 
>> operations is to use the rounding mode and exception handling metadata to 
>> mark constrained intrinsics as having LLVM’s default behavior.

@sepavloff please can you undo the clang part of this change (+@hans) and post 
an RFC to further hash out the design here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Thomas Preud'homme via Phabricator via cfe-commits]

thopre added a comment.

In D104854#2957735 , @kpn wrote:

> In D104854#2957490 , @lebedev.ri 
> wrote:
>
>> In D104854#2957471 , @sepavloff 
>> wrote:
>>
>>> In D104854#2957423 , @spatel 
>>> wrote:
>>>
 Is it intentional that we are not canonicalizing the intrinsic call back 
 to `fcmp uno` in the default FP environment?
>>>
>>> It is lowered to unordered comparison by default. Changing `llvm.isnan` to  
>>> `fcmp uno` somewhere in IR would make it possible to optimize out the 
>>> latter if fast-math mode is on. Preserving semantics of `isnan` when 
>>> fast-math is in effect was one of the goals of this change.
>>
>> Eeek. Was there an RFC about this?
>> This does not sound good to me at all,
>> much like "let's not apply fast-math flags to x86 vector intrinsics".
>
> We can switch into and out of the default FP environment inside a single 
> function.

Really? The constrained intrinsic documentation claims the reverse 
(https://llvm.org/docs/LangRef.html#constrainedfp):

> If any FP operation in a function is constrained then they all must be 
> constrained. This is required for correct LLVM IR. Optimizations that move 
> code around can create miscompiles if mixing of constrained and normal 
> operations is done. The correct way to mix constrained and less constrained 
> operations is to use the rounding mode and exception handling metadata to 
> mark constrained intrinsics as having LLVM’s default behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

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

kpn added a comment.

In D104854#2957490 , @lebedev.ri 
wrote:

> In D104854#2957471 , @sepavloff 
> wrote:
>
>> In D104854#2957423 , @spatel wrote:
>>
>>> Is it intentional that we are not canonicalizing the intrinsic call back to 
>>> `fcmp uno` in the default FP environment?
>>
>> It is lowered to unordered comparison by default. Changing `llvm.isnan` to  
>> `fcmp uno` somewhere in IR would make it possible to optimize out the latter 
>> if fast-math mode is on. Preserving semantics of `isnan` when fast-math is 
>> in effect was one of the goals of this change.
>
> Eeek. Was there an RFC about this?
> This does not sound good to me at all,
> much like "let's not apply fast-math flags to x86 vector intrinsics".

We can switch into and out of the default FP environment inside a single 
function. If we want different behavior based on the FP environment then this 
should be a constrained intrinsic. Then the intrinsic would know the FP 
environment, or at least enough about it to know if traps and FP status bits 
are relevant.

I think the distinction is constrained vs non-constrained because FMF can 
optionally be used in both cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Sanjay Patel via Phabricator via cfe-commits]

spatel added a comment.

In D104854#2957582 , @lebedev.ri 
wrote:

> In D104854#2957529 , @spatel wrote:
>
>> In D104854#2957471 , @sepavloff 
>> wrote:
>>
>>> In D104854#2957423 , @spatel 
>>> wrote:
>>>
 Is it intentional that we are not canonicalizing the intrinsic call back 
 to `fcmp uno` in the default FP environment?
>>>
>>> It is lowered to unordered comparison by default. Changing `llvm.isnan` to  
>>> `fcmp uno` somewhere in IR would make it possible to optimize out the 
>>> latter if fast-math mode is on. Preserving semantics of `isnan` when 
>>> fast-math is in effect was one of the goals of this change.
>>
>> I understand that the codegen was supposed to be no worse, but the 
>> difference in IR causes optimizer regressions like: 
>> https://llvm.org/PR51556
>>
>> If we want this intrinsic (and its siblings that haven't been created yet) 
>> to survive through IR, then we have to enhance IR passes to recognize the 
>> new patterns. 
>> It would be easier to do this in steps: (1) create the intrinsic only if not 
>> in the default FP env, (2) update IR analysis/passes to recognize the 
>> intrinsic, (3) create the intrinsic in the default FP env with no FMF, (4) 
>> create the intrinsic always.
>
> +1, but right now i'm not sold on the behavior of not optimizing away NaN 
> checks in no-NaN's mode.
> At least that part should be reconciled now. It *might* be an improvement, 
> but it caters to expectations
> of one group while catering away from the documentation and existing 
> expectations of other groups.
> This shouldn't be decided in a review, it should be driven by an RFC.

Agree. I think a revert followed by RFC to make sure there is consensus on 
semantics is needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D104854#2957529 , @spatel wrote:

> In D104854#2957471 , @sepavloff 
> wrote:
>
>> In D104854#2957423 , @spatel wrote:
>>
>>> Is it intentional that we are not canonicalizing the intrinsic call back to 
>>> `fcmp uno` in the default FP environment?
>>
>> It is lowered to unordered comparison by default. Changing `llvm.isnan` to  
>> `fcmp uno` somewhere in IR would make it possible to optimize out the latter 
>> if fast-math mode is on. Preserving semantics of `isnan` when fast-math is 
>> in effect was one of the goals of this change.
>
> I understand that the codegen was supposed to be no worse, but the difference 
> in IR causes optimizer regressions like: 
> https://llvm.org/PR51556
>
> If we want this intrinsic (and its siblings that haven't been created yet) to 
> survive through IR, then we have to enhance IR passes to recognize the new 
> patterns. 
> It would be easier to do this in steps: (1) create the intrinsic only if not 
> in the default FP env, (2) update IR analysis/passes to recognize the 
> intrinsic, (3) create the intrinsic in the default FP env with no FMF, (4) 
> create the intrinsic always.

+1, but right now i'm not sold on the behavior of not optimizing away NaN 
checks in no-NaN's mode.
At least that part should be reconciled now. It *might* be an improvement, but 
it caters to expectations
of one group while catering away from the documentation and existing 
expectations of other groups.
This shouldn't be decided in a review, it should be driven by an RFC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

In D104854#2957529 , @spatel wrote:

> In D104854#2957471 , @sepavloff 
> wrote:
>
>> In D104854#2957423 , @spatel wrote:
>>
>>> Is it intentional that we are not canonicalizing the intrinsic call back to 
>>> `fcmp uno` in the default FP environment?
>>
>> It is lowered to unordered comparison by default. Changing `llvm.isnan` to  
>> `fcmp uno` somewhere in IR would make it possible to optimize out the latter 
>> if fast-math mode is on. Preserving semantics of `isnan` when fast-math is 
>> in effect was one of the goals of this change.
>
> I understand that the codegen was supposed to be no worse, but the difference 
> in IR causes optimizer regressions like: 
> https://llvm.org/PR51556
>
> If we want this intrinsic (and its siblings that haven't been created yet) to 
> survive through IR, then we have to enhance IR passes to recognize the new 
> patterns. 
> It would be easier to do this in steps: (1) create the intrinsic only if not 
> in the default FP env, (2) update IR analysis/passes to recognize the 
> intrinsic, (3) create the intrinsic in the default FP env with no FMF, (4) 
> create the intrinsic always.

Meanwhile this should be reverted..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Sanjay Patel via Phabricator via cfe-commits]

spatel added a comment.

In D104854#2957471 , @sepavloff wrote:

> In D104854#2957423 , @spatel wrote:
>
>> Is it intentional that we are not canonicalizing the intrinsic call back to 
>> `fcmp uno` in the default FP environment?
>
> It is lowered to unordered comparison by default. Changing `llvm.isnan` to  
> `fcmp uno` somewhere in IR would make it possible to optimize out the latter 
> if fast-math mode is on. Preserving semantics of `isnan` when fast-math is in 
> effect was one of the goals of this change.

I understand that the codegen was supposed to be no worse, but the difference 
in IR causes optimizer regressions like: 
https://llvm.org/PR51556

If we want this intrinsic (and its siblings that haven't been created yet) to 
survive through IR, then we have to enhance IR passes to recognize the new 
patterns. 
It would be easier to do this in steps: (1) create the intrinsic only if not in 
the default FP env, (2) update IR analysis/passes to recognize the intrinsic, 
(3) create the intrinsic in the default FP env with no FMF, (4) create the 
intrinsic always.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D104854#2957471 , @sepavloff wrote:

> In D104854#2957423 , @spatel wrote:
>
>> Is it intentional that we are not canonicalizing the intrinsic call back to 
>> `fcmp uno` in the default FP environment?
>
> It is lowered to unordered comparison by default. Changing `llvm.isnan` to  
> `fcmp uno` somewhere in IR would make it possible to optimize out the latter 
> if fast-math mode is on. Preserving semantics of `isnan` when fast-math is in 
> effect was one of the goals of this change.

Eeek. Was there an RFC about this?
This does not sound good to me at all,
much like "let's not apply fast-math flags to x86 vector intrinsics".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D104854#2957423 , @spatel wrote:

> Is it intentional that we are not canonicalizing the intrinsic call back to 
> `fcmp uno` in the default FP environment?

It is lowered to unordered comparison by default. Changing `llvm.isnan` to  
`fcmp uno` somewhere in IR would make it possible to optimize out the latter if 
fast-math mode is on. Preserving semantics of `isnan` when fast-math is in 
effect was one of the goals of this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Sanjay Patel via Phabricator via cfe-commits]

spatel added a comment.

Is it intentional that we are not canonicalizing the intrinsic call back to 
`fcmp uno` in the default FP environment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

Patch D108037  must make lowering of 
`llvm.isnan` more close to the code produced by `__builtin_isnan` earlier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Siva Chandra via Phabricator via cfe-commits]

sivachandra added inline comments.



Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22193
+DAG.getConstant(0x45, DL, MVT::i8));
+
+  return DAG.getSetCC(DL, ResultVT, Extract, DAG.getConstant(1, DL, MVT::i8),

While I do not understand the code mechanics of this patch, I am mostly in 
agreement with the general direction of this patch. However, it has lead to a 
change in behavior wrt 80-bit x86 floating point numbers. Unlike the 32-bit and 
64-bit floating point numbers, 80-bit numbers have an additional class of 
"Unsupported Numbers". Those numbers were previously treated as NaNs. Since 
this change uses the `fxam` instruction to classify the input number, that is 
not the case any more as the `fxam` instruction distinguishes between 
unsupported numbers and NaNs. So, to restore the previous behavior, can we 
extend this patch to treat unsupported numbers as NaNs? At a high level, what I 
am effectively saying is that we should implement `isnan` this way:

```
bool isnan(long double x) {
  uint16_t status;
  __asm__ __volatile__("fldt %0" : : "m"(x));
  __asm__ __volatile__("fxam");
  __asm__ __volatile__("fnstsw %0": "=m"(status):);
  uint16_t c0c2c3 = (status >> 8) & 0x45;
  return c0c2c3 <= 1; // This patch seems to be only doing c0c2c3 == 1 check.
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added inline comments.



Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6964
+return DAG.getSetCC(DL, ResultVT, Op, DAG.getConstantFP(0.0, DL, 
OperandVT),
+ISD::SETUO);
+

nemanjai wrote:
> sepavloff wrote:
> > nemanjai wrote:
> > > sepavloff wrote:
> > > > efriedma wrote:
> > > > > Maybe we want to consider falling back to the integer path if SETCC 
> > > > > isn't legal for the given operand type?  We could do that as a 
> > > > > followup, though.
> > > > It makes sense, it could be beneficial for targets that have limited 
> > > > set of floating point comparisons. However straightforward check like:
> > > > 
> > > > if (Flags.hasNoFPExcept() && isOperationLegalOrCustom(ISD::SETCC, 
> > > > OperandVT))
> > > > 
> > > > results in worse code, mainly for vector types. It should be more 
> > > > complex check.
> > > >  
> > > Out of curiosity, why was this added when you recognized that it results 
> > > in worse code? This is certainly part of the reason for the regression 
> > > for `ppc_fp128`.
> > > 
> > > It would appear that before this patch, we would emit a comparison for 
> > > all types that are not IEEE FP types (such as `ppc_fp128`). Those 
> > > semantics do not seem to have carried over.
> > > Out of curiosity, why was this added when you recognized that it results 
> > > in worse code? This is certainly part of the reason for the regression 
> > > for ppc_fp128.
> > 
> > It is my mistake. After experiments I forgot to remove this change. I am 
> > sorry.
> > 
> > For x86 and AArch64 I used modified `test-suite`, with changes from 
> > D106804. Without proper tests it is hard to reveal why one intrinsic starts 
> > to fail.
> > 
> > > It would appear that before this patch, we would emit a comparison for 
> > > all types that are not IEEE FP types (such as ppc_fp128). Those semantics 
> > > do not seem to have carried over.
> > 
> > The previous behavior is not correct in non-default FP environment. 
> > Unordered comparison raises Invalid exception if an operand is signaling 
> > NaN. On the other hand, `isnan` must never raise exceptions.
> Well, if the **must never raise exceptions** is an IEEE-754 requirement (i.e. 
> as noted in 5.7.2), I think it is reasonable that operations on types that do 
> not conform to IEEE-754 are not bound by it.
C standard defines macro `isnan`, of which the recent draft 
(http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2596.pdf, F.3p6) states:

The C classification macros fpclassify, iscanonical, isfinite, isinf, 
isnan, isnormal,
issignaling, issubnormal, and iszero provide the IEC 60559 operations 
indicated in the table above 
provided their arguments are in the format of their semantic type. Then 
these macros
raise no floating-point exceptions, even if an argument is a signaling NaN.

This statement is not restricted to IEEE-compatible types, so any floating 
point type must behave according to this statement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added inline comments.



Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6964
+return DAG.getSetCC(DL, ResultVT, Op, DAG.getConstantFP(0.0, DL, 
OperandVT),
+ISD::SETUO);
+

sepavloff wrote:
> nemanjai wrote:
> > sepavloff wrote:
> > > efriedma wrote:
> > > > Maybe we want to consider falling back to the integer path if SETCC 
> > > > isn't legal for the given operand type?  We could do that as a 
> > > > followup, though.
> > > It makes sense, it could be beneficial for targets that have limited set 
> > > of floating point comparisons. However straightforward check like:
> > > 
> > > if (Flags.hasNoFPExcept() && isOperationLegalOrCustom(ISD::SETCC, 
> > > OperandVT))
> > > 
> > > results in worse code, mainly for vector types. It should be more complex 
> > > check.
> > >  
> > Out of curiosity, why was this added when you recognized that it results in 
> > worse code? This is certainly part of the reason for the regression for 
> > `ppc_fp128`.
> > 
> > It would appear that before this patch, we would emit a comparison for all 
> > types that are not IEEE FP types (such as `ppc_fp128`). Those semantics do 
> > not seem to have carried over.
> > Out of curiosity, why was this added when you recognized that it results in 
> > worse code? This is certainly part of the reason for the regression for 
> > ppc_fp128.
> 
> It is my mistake. After experiments I forgot to remove this change. I am 
> sorry.
> 
> For x86 and AArch64 I used modified `test-suite`, with changes from D106804. 
> Without proper tests it is hard to reveal why one intrinsic starts to fail.
> 
> > It would appear that before this patch, we would emit a comparison for all 
> > types that are not IEEE FP types (such as ppc_fp128). Those semantics do 
> > not seem to have carried over.
> 
> The previous behavior is not correct in non-default FP environment. Unordered 
> comparison raises Invalid exception if an operand is signaling NaN. On the 
> other hand, `isnan` must never raise exceptions.
Well, if the **must never raise exceptions** is an IEEE-754 requirement (i.e. 
as noted in 5.7.2), I think it is reasonable that operations on types that do 
not conform to IEEE-754 are not bound by it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D104854#2932444 , @nemanjai wrote:

> Rather than reverting this commit again, I pushed 62fe3dcf98d1 
>  to use 
> the same expansion as before (using unordered comparison) for `ppc_fp128`.

Thank you very much for fixing that!

> I am not sure if there are other types that suffer from the same problem 
> (perhaps the Intel 80-bit long double).

Intel `fp80` is also non-IEEE type but it got custom lowering in this patch. 
There is little chance for such type to work properly without custom lowering.

I am working on patch that would add custom lowering of `llvm.isnan` to PowerPC.




Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6964
+return DAG.getSetCC(DL, ResultVT, Op, DAG.getConstantFP(0.0, DL, 
OperandVT),
+ISD::SETUO);
+

nemanjai wrote:
> sepavloff wrote:
> > efriedma wrote:
> > > Maybe we want to consider falling back to the integer path if SETCC isn't 
> > > legal for the given operand type?  We could do that as a followup, though.
> > It makes sense, it could be beneficial for targets that have limited set of 
> > floating point comparisons. However straightforward check like:
> > 
> > if (Flags.hasNoFPExcept() && isOperationLegalOrCustom(ISD::SETCC, 
> > OperandVT))
> > 
> > results in worse code, mainly for vector types. It should be more complex 
> > check.
> >  
> Out of curiosity, why was this added when you recognized that it results in 
> worse code? This is certainly part of the reason for the regression for 
> `ppc_fp128`.
> 
> It would appear that before this patch, we would emit a comparison for all 
> types that are not IEEE FP types (such as `ppc_fp128`). Those semantics do 
> not seem to have carried over.
> Out of curiosity, why was this added when you recognized that it results in 
> worse code? This is certainly part of the reason for the regression for 
> ppc_fp128.

It is my mistake. After experiments I forgot to remove this change. I am sorry.

For x86 and AArch64 I used modified `test-suite`, with changes from D106804. 
Without proper tests it is hard to reveal why one intrinsic starts to fail.

> It would appear that before this patch, we would emit a comparison for all 
> types that are not IEEE FP types (such as ppc_fp128). Those semantics do not 
> seem to have carried over.

The previous behavior is not correct in non-default FP environment. Unordered 
comparison raises Invalid exception if an operand is signaling NaN. On the 
other hand, `isnan` must never raise exceptions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

Rather than reverting this commit again, I pushed 62fe3dcf98d1 
 to use 
the same expansion as before (using unordered comparison) for `ppc_fp128`. I am 
not sure if there are other types that suffer from the same problem (perhaps 
the Intel 80-bit long double).




Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6964
+return DAG.getSetCC(DL, ResultVT, Op, DAG.getConstantFP(0.0, DL, 
OperandVT),
+ISD::SETUO);
+

sepavloff wrote:
> efriedma wrote:
> > Maybe we want to consider falling back to the integer path if SETCC isn't 
> > legal for the given operand type?  We could do that as a followup, though.
> It makes sense, it could be beneficial for targets that have limited set of 
> floating point comparisons. However straightforward check like:
> 
> if (Flags.hasNoFPExcept() && isOperationLegalOrCustom(ISD::SETCC, 
> OperandVT))
> 
> results in worse code, mainly for vector types. It should be more complex 
> check.
>  
Out of curiosity, why was this added when you recognized that it results in 
worse code? This is certainly part of the reason for the regression for 
`ppc_fp128`.

It would appear that before this patch, we would emit a comparison for all 
types that are not IEEE FP types (such as `ppc_fp128`). Those semantics do not 
seem to have carried over.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D104854#2932186 , @nemanjai wrote:

> This appears to have caused some failures on PPC buildbots. For example: 
> https://lab.llvm.org/buildbot/#/builders/105/builds/13446
> We are investigating this. Can you please pull this to bring the bots back to 
> green until we track down the reason for the problem and can provide a fix?

Sorry, I just realized you had pulled it just prior to me posting this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

This appears to have caused some failures on PPC buildbots. For example: 
https://lab.llvm.org/buildbot/#/builders/105/builds/13446
We are investigating this. Can you please pull this to bring the bots back to 
green until we track down the reason for the problem and can provide a fix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Eli Friedman via Phabricator via cfe-commits]

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

LGTM.  (Since there have been a bunch of reviewers involved, please give a few 
days before you merge.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added inline comments.



Comment at: llvm/test/Transforms/InstCombine/fpclass.ll:29
+  ret <2 x i1> %t
+}
+

RKSimon wrote:
> You probably need some negative tests (no flags, ninf instead of nnan etc.)?
Added few such tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added inline comments.



Comment at: llvm/test/Transforms/InstCombine/fpclass.ll:29
+  ret <2 x i1> %t
+}
+

You probably need some negative tests (no flags, ninf instead of nnan etc.)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added inline comments.



Comment at: llvm/test/CodeGen/X86/x86-fpclass.ll:173
+
+define <1 x i1> @isnan_double_vec1(<1 x double> %x) {
+; CHECK-32-LABEL: isnan_double_vec1:

RKSimon wrote:
> add nounwind to reduce cfi noise (other tests would benefit as well)?
Good hint, thank you!



Comment at: llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll:1
+; RUN: opt < %s -instsimplify -S | FileCheck %s
+

RKSimon wrote:
> Use update_test_checks.py?
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added inline comments.



Comment at: llvm/test/CodeGen/X86/x86-fpclass.ll:173
+
+define <1 x i1> @isnan_double_vec1(<1 x double> %x) {
+; CHECK-32-LABEL: isnan_double_vec1:

add nounwind to reduce cfi noise (other tests would benefit as well)?



Comment at: llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll:1
+; RUN: opt < %s -instsimplify -S | FileCheck %s
+

Use update_test_checks.py?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D104854#2905430 , @efriedma wrote:

> In D104854#2904826 , @sepavloff 
> wrote:
>
>> In D104854#2886328 , @efriedma 
>> wrote:
>>
 The options '-ffast-math' and '-fno-honor-nans' imply that FP operation
 operands are never NaNs. This assumption however should not be applied
 to the functions that check FP number properties, including 'isnan'. If
 such function returns expected result instead of actually making
 checks, it becomes useless in many cases.
>>>
>>> This doesn't work the way you want it to, at least given the way nnan/ninf 
>>> are currently defined in LangRef.  It's possible to end up in a situation 
>>> where `isnan(x) == isnan(x)` evaluates to false at runtime.  It doesn't 
>>> matter how you compute isnan; the problem is that the input is poisoned.
>>>
>>> I think the right solution to this sort of issue is to insert a "freeze" in 
>>> the LLVM IR, or something like that.  Not sure how we'd expect users to 
>>> write this in C.  Suggestions welcome.
>>
>> According to the documentation, nnan/ninf may be applied to `fneg`, `fadd`, 
>> `fsub`, `fmul`, `fdiv`, `frem`, `fcmp`, `phi`, `select` and `call`. We can 
>> ignore this flag for calls of isnan and similar functions. Of course, if 
>> conditions of using `-ffast-math` are broken, we have undefined behavior and 
>> `isnan(x) != isnan(x)` becomes possible, like in this code:
>
> Right... so how can you produce a NaN in these circumstances?  You could load 
> one from memory, I guess?

Yes, they come from structures in memory. I think they can also come from 
function arguments, if some source files are compiled with option `-ffast-math` 
and some without.

> It would probably be a good idea to have an instcombine that combines away 
> isnan on a value produced by an operation marked nnan, so we don't confuse 
> people reading assembly into assuming isnan is actually reliable in that 
> context.

Added such transformation (file 
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp, test in 
llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

In D104854#2904826 , @sepavloff wrote:

> In D104854#2886328 , @efriedma 
> wrote:
>
>>> The options '-ffast-math' and '-fno-honor-nans' imply that FP operation
>>> operands are never NaNs. This assumption however should not be applied
>>> to the functions that check FP number properties, including 'isnan'. If
>>> such function returns expected result instead of actually making
>>> checks, it becomes useless in many cases.
>>
>> This doesn't work the way you want it to, at least given the way nnan/ninf 
>> are currently defined in LangRef.  It's possible to end up in a situation 
>> where `isnan(x) == isnan(x)` evaluates to false at runtime.  It doesn't 
>> matter how you compute isnan; the problem is that the input is poisoned.
>>
>> I think the right solution to this sort of issue is to insert a "freeze" in 
>> the LLVM IR, or something like that.  Not sure how we'd expect users to 
>> write this in C.  Suggestions welcome.
>
> According to the documentation, nnan/ninf may be applied to `fneg`, `fadd`, 
> `fsub`, `fmul`, `fdiv`, `frem`, `fcmp`, `phi`, `select` and `call`. We can 
> ignore this flag for calls of isnan and similar functions. Of course, if 
> conditions of using `-ffast-math` are broken, we have undefined behavior and 
> `isnan(x) != isnan(x)` becomes possible, like in this code:

Right... so how can you produce a NaN in these circumstances?  You could load 
one from memory, I guess?

It would probably be a good idea to have an instcombine that combines away 
isnan on a value produced by an operation marked nnan, so we don't confuse 
people reading assembly into assuming isnan is actually reliable in that 
context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added inline comments.



Comment at: llvm/docs/LangRef.rst:20983
+
+These functions get properties of floating point values.
+

tschuett wrote:
> 
Fixed, thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff updated this revision to Diff 361718.
sepavloff added a comment.

Fixed documentation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/X86/strictfp_builtins.c
  clang/test/CodeGen/aarch64-strictfp-builtins.c
  clang/test/CodeGen/strictfp_builtins.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/ISDOpcodes.h
  llvm/include/llvm/CodeGen/TargetLowering.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Analysis/ConstantFolding.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
  llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
  llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
  llvm/lib/CodeGen/TargetLoweringBase.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/test/CodeGen/AArch64/aarch64-fpclass.ll
  llvm/test/CodeGen/PowerPC/ppc-fpclass.ll
  llvm/test/CodeGen/X86/x86-fpclass.ll
  llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll

Index: llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll
@@ -0,0 +1,28 @@
+; RUN: opt < %s -instsimplify -S | FileCheck %s
+
+define i1 @isnan_01() {
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float 0x7FF8)
+  ret i1 %0
+}
+; CHECK-LABEL: isnan_01
+; CHECK:   ret i1 true 
+
+define i1 @isnan_02() {
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float 0x7FF0)
+  ret i1 %0
+}
+; CHECK-LABEL: isnan_02
+; CHECK:   ret i1 false 
+
+define <4 x i1> @isnan_03() {
+entry:
+  %0 = tail call <4 x i1> @llvm.isnan.v4f32(<4 x float>)
+  ret <4 x i1> %0
+}
+; CHECK-LABEL: isnan_03
+; CHECK:   ret <4 x i1> 
+
+declare i1 @llvm.isnan.f32(float)
+declare <4 x i1> @llvm.isnan.v4f32(<4 x float>)
Index: llvm/test/CodeGen/X86/x86-fpclass.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/x86-fpclass.ll
@@ -0,0 +1,641 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=i686 | FileCheck %s -check-prefix=CHECK-32
+; RUN: llc < %s -mtriple=x86_64 | FileCheck %s -check-prefix=CHECK-64
+
+define i1 @isnan_float(float %x) {
+; CHECK-32-LABEL: isnan_float:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:flds {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fucomp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:# kill: def $ah killed $ah killed $ax
+; CHECK-32-NEXT:sahf
+; CHECK-32-NEXT:setp %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_float:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:ucomiss %xmm0, %xmm0
+; CHECK-64-NEXT:setp %al
+; CHECK-64-NEXT:retq
+; NOSSE-32-LABEL: isnan_float:
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float %x)
+  ret i1 %0
+}
+
+define i1 @isnan_double(double %x) {
+; CHECK-32-LABEL: isnan_double:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:fldl {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fucomp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:# kill: def $ah killed $ah killed $ax
+; CHECK-32-NEXT:sahf
+; CHECK-32-NEXT:setp %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_double:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:ucomisd %xmm0, %xmm0
+; CHECK-64-NEXT:setp %al
+; CHECK-64-NEXT:retq
+entry:
+  %0 = tail call i1 @llvm.isnan.f64(double %x)
+  ret i1 %0
+}
+
+define i1 @isnan_ldouble(x86_fp80 %x) {
+; CHECK-32-LABEL: isnan_ldouble:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:fldt {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fxam
+; CHECK-32-NEXT:fstp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:andb $69, %ah
+; CHECK-32-NEXT:cmpb $1, %ah
+; CHECK-32-NEXT:sete %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_ldouble:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:fldt {{[0-9]+}}(%rsp)
+; CHECK-64-NEXT:fxam
+; CHECK-64-NEXT:fstp %st(0)
+; CHECK-64-NEXT:fnstsw %ax
+; CHECK-64-NEXT:andb $69, %ah
+; CHECK-64-NEXT:cmpb $1, %ah
+; CHECK-64-NEXT:sete %al
+; CHECK-64-NEXT:retq
+entry:
+  %0 = tail call i1 @llvm.isnan.f80(x86_fp80 %x)
+  ret i1 %0
+}
+
+define i1 @isnan_float_strictfp(float %x) strictfp {
+; CHECK-32-LABEL: isnan_float_strictfp:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:movl $2147483647, %eax # imm = 0x7FFF
+; CHECK-32-NEXT:andl {{[0-9]+}}(%esp), %eax
+; CHECK-32-NEXT:cmpl $2139095041, %eax # imm = 0x7F81
+; CHECK-32-NEXT:setge %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_float_strictfp:
+; CHECK-64:   # %bb.0: # %entry
+; 

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D104854#2886328 , @efriedma wrote:

>> The options '-ffast-math' and '-fno-honor-nans' imply that FP operation
>> operands are never NaNs. This assumption however should not be applied
>> to the functions that check FP number properties, including 'isnan'. If
>> such function returns expected result instead of actually making
>> checks, it becomes useless in many cases.
>
> This doesn't work the way you want it to, at least given the way nnan/ninf 
> are currently defined in LangRef.  It's possible to end up in a situation 
> where `isnan(x) == isnan(x)` evaluates to false at runtime.  It doesn't 
> matter how you compute isnan; the problem is that the input is poisoned.
>
> I think the right solution to this sort of issue is to insert a "freeze" in 
> the LLVM IR, or something like that.  Not sure how we'd expect users to write 
> this in C.  Suggestions welcome.

According to the documentation, nnan/ninf may be applied to `fneg`, `fadd`, 
`fsub`, `fmul`, `fdiv`, `frem`, `fcmp`, `phi`, `select` and `call`. We can 
ignore this flag for calls of isnan and similar functions. Of course, if 
conditions of using `-ffast-math` are broken, we have undefined behavior and 
`isnan(x) != isnan(x)` becomes possible, like in this code:

  %c = fadd %a, nan
  %r = call llvm.isnan.f32(%c) 

Similarly, it is legitimate to optimize `isnan` in the code:

  %c = fadd %a, %b
  %r = call llvm.isnan.f32(%c)

In this case the result of `fadd` cannot be NaN, otherwise contract of 
`-ffast-math` is broken. So `isnan` in this case may be optimized to `false`.




Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6964
+return DAG.getSetCC(DL, ResultVT, Op, DAG.getConstantFP(0.0, DL, 
OperandVT),
+ISD::SETUO);
+

efriedma wrote:
> Maybe we want to consider falling back to the integer path if SETCC isn't 
> legal for the given operand type?  We could do that as a followup, though.
It makes sense, it could be beneficial for targets that have limited set of 
floating point comparisons. However straightforward check like:

if (Flags.hasNoFPExcept() && isOperationLegalOrCustom(ISD::SETCC, 
OperandVT))

results in worse code, mainly for vector types. It should be more complex check.
 



Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6984
+  return DAG.getSetCC(DL, ResultVT, Sub, DAG.getConstant(0, DL, IntVT),
+  ISD::SETNE);
+}

efriedma wrote:
> Instead of emitting `ExpMaskV - AbsV != 0`, can we just emit `ExpMaskV != 
> AbsV`?
> Instead of emitting ExpMaskV - AbsV != 0, can we just emit ExpMaskV != AbsV?

Implemented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Thorsten via Phabricator via cfe-commits]

tschuett added inline comments.



Comment at: llvm/docs/LangRef.rst:20983
+
+These functions get properties of floating point values.
+




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff updated this revision to Diff 361707.
sepavloff added a comment.

Updated patch

- Rebased,
- Applied small enhancement to integer implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/X86/strictfp_builtins.c
  clang/test/CodeGen/aarch64-strictfp-builtins.c
  clang/test/CodeGen/strictfp_builtins.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/ISDOpcodes.h
  llvm/include/llvm/CodeGen/TargetLowering.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Analysis/ConstantFolding.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
  llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
  llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
  llvm/lib/CodeGen/TargetLoweringBase.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/test/CodeGen/AArch64/aarch64-fpclass.ll
  llvm/test/CodeGen/PowerPC/ppc-fpclass.ll
  llvm/test/CodeGen/X86/x86-fpclass.ll
  llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll

Index: llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll
@@ -0,0 +1,28 @@
+; RUN: opt < %s -instsimplify -S | FileCheck %s
+
+define i1 @isnan_01() {
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float 0x7FF8)
+  ret i1 %0
+}
+; CHECK-LABEL: isnan_01
+; CHECK:   ret i1 true 
+
+define i1 @isnan_02() {
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float 0x7FF0)
+  ret i1 %0
+}
+; CHECK-LABEL: isnan_02
+; CHECK:   ret i1 false 
+
+define <4 x i1> @isnan_03() {
+entry:
+  %0 = tail call <4 x i1> @llvm.isnan.v4f32(<4 x float>)
+  ret <4 x i1> %0
+}
+; CHECK-LABEL: isnan_03
+; CHECK:   ret <4 x i1> 
+
+declare i1 @llvm.isnan.f32(float)
+declare <4 x i1> @llvm.isnan.v4f32(<4 x float>)
Index: llvm/test/CodeGen/X86/x86-fpclass.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/x86-fpclass.ll
@@ -0,0 +1,641 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=i686 | FileCheck %s -check-prefix=CHECK-32
+; RUN: llc < %s -mtriple=x86_64 | FileCheck %s -check-prefix=CHECK-64
+
+define i1 @isnan_float(float %x) {
+; CHECK-32-LABEL: isnan_float:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:flds {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fucomp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:# kill: def $ah killed $ah killed $ax
+; CHECK-32-NEXT:sahf
+; CHECK-32-NEXT:setp %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_float:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:ucomiss %xmm0, %xmm0
+; CHECK-64-NEXT:setp %al
+; CHECK-64-NEXT:retq
+; NOSSE-32-LABEL: isnan_float:
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float %x)
+  ret i1 %0
+}
+
+define i1 @isnan_double(double %x) {
+; CHECK-32-LABEL: isnan_double:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:fldl {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fucomp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:# kill: def $ah killed $ah killed $ax
+; CHECK-32-NEXT:sahf
+; CHECK-32-NEXT:setp %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_double:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:ucomisd %xmm0, %xmm0
+; CHECK-64-NEXT:setp %al
+; CHECK-64-NEXT:retq
+entry:
+  %0 = tail call i1 @llvm.isnan.f64(double %x)
+  ret i1 %0
+}
+
+define i1 @isnan_ldouble(x86_fp80 %x) {
+; CHECK-32-LABEL: isnan_ldouble:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:fldt {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fxam
+; CHECK-32-NEXT:fstp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:andb $69, %ah
+; CHECK-32-NEXT:cmpb $1, %ah
+; CHECK-32-NEXT:sete %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_ldouble:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:fldt {{[0-9]+}}(%rsp)
+; CHECK-64-NEXT:fxam
+; CHECK-64-NEXT:fstp %st(0)
+; CHECK-64-NEXT:fnstsw %ax
+; CHECK-64-NEXT:andb $69, %ah
+; CHECK-64-NEXT:cmpb $1, %ah
+; CHECK-64-NEXT:sete %al
+; CHECK-64-NEXT:retq
+entry:
+  %0 = tail call i1 @llvm.isnan.f80(x86_fp80 %x)
+  ret i1 %0
+}
+
+define i1 @isnan_float_strictfp(float %x) strictfp {
+; CHECK-32-LABEL: isnan_float_strictfp:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:movl $2147483647, %eax # imm = 0x7FFF
+; CHECK-32-NEXT:andl {{[0-9]+}}(%esp), %eax
+; CHECK-32-NEXT:cmpl $2139095041, %eax # imm = 0x7F81
+; CHECK-32-NEXT:setge %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: 

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> The options '-ffast-math' and '-fno-honor-nans' imply that FP operation
> operands are never NaNs. This assumption however should not be applied
> to the functions that check FP number properties, including 'isnan'. If
> such function returns expected result instead of actually making
> checks, it becomes useless in many cases.

This doesn't work the way you want it to, at least given the way nnan/ninf are 
currently defined in LangRef.  It's possible to end up in a situation where 
`isnan(x) == isnan(x)` evaluates to false at runtime.  It doesn't matter how 
you compute isnan; the problem is that the input is poisoned.

I think the right solution to this sort of issue is to insert a "freeze" in the 
LLVM IR, or something like that.  Not sure how we'd expect users to write this 
in C.  Suggestions welcome.




Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6964
+return DAG.getSetCC(DL, ResultVT, Op, DAG.getConstantFP(0.0, DL, 
OperandVT),
+ISD::SETUO);
+

Maybe we want to consider falling back to the integer path if SETCC isn't legal 
for the given operand type?  We could do that as a followup, though.



Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6984
+  return DAG.getSetCC(DL, ResultVT, Sub, DAG.getConstant(0, DL, IntVT),
+  ISD::SETNE);
+}

Instead of emitting `ExpMaskV - AbsV != 0`, can we just emit `ExpMaskV != AbsV`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff updated this revision to Diff 357857.
sepavloff added a comment.

Rebased


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/X86/strictfp_builtins.c
  clang/test/CodeGen/aarch64-strictfp-builtins.c
  clang/test/CodeGen/strictfp_builtins.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/ISDOpcodes.h
  llvm/include/llvm/CodeGen/TargetLowering.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Analysis/ConstantFolding.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
  llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
  llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
  llvm/lib/CodeGen/TargetLoweringBase.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/test/CodeGen/AArch64/aarch64-fpclass.ll
  llvm/test/CodeGen/PowerPC/ppc-fpclass.ll
  llvm/test/CodeGen/X86/x86-fpclass.ll
  llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll

Index: llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll
@@ -0,0 +1,28 @@
+; RUN: opt < %s -instsimplify -S | FileCheck %s
+
+define i1 @isnan_01() {
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float 0x7FF8)
+  ret i1 %0
+}
+; CHECK-LABEL: isnan_01
+; CHECK:   ret i1 true 
+
+define i1 @isnan_02() {
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float 0x7FF0)
+  ret i1 %0
+}
+; CHECK-LABEL: isnan_02
+; CHECK:   ret i1 false 
+
+define <4 x i1> @isnan_03() {
+entry:
+  %0 = tail call <4 x i1> @llvm.isnan.v4f32(<4 x float>)
+  ret <4 x i1> %0
+}
+; CHECK-LABEL: isnan_03
+; CHECK:   ret <4 x i1> 
+
+declare i1 @llvm.isnan.f32(float)
+declare <4 x i1> @llvm.isnan.v4f32(<4 x float>)
Index: llvm/test/CodeGen/X86/x86-fpclass.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/x86-fpclass.ll
@@ -0,0 +1,655 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=i686 | FileCheck %s -check-prefix=CHECK-32
+; RUN: llc < %s -mtriple=x86_64 | FileCheck %s -check-prefix=CHECK-64
+
+define i1 @isnan_float(float %x) {
+; CHECK-32-LABEL: isnan_float:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:flds {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fucomp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:# kill: def $ah killed $ah killed $ax
+; CHECK-32-NEXT:sahf
+; CHECK-32-NEXT:setp %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_float:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:ucomiss %xmm0, %xmm0
+; CHECK-64-NEXT:setp %al
+; CHECK-64-NEXT:retq
+; NOSSE-32-LABEL: isnan_float:
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float %x)
+  ret i1 %0
+}
+
+define i1 @isnan_double(double %x) {
+; CHECK-32-LABEL: isnan_double:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:fldl {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fucomp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:# kill: def $ah killed $ah killed $ax
+; CHECK-32-NEXT:sahf
+; CHECK-32-NEXT:setp %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_double:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:ucomisd %xmm0, %xmm0
+; CHECK-64-NEXT:setp %al
+; CHECK-64-NEXT:retq
+entry:
+  %0 = tail call i1 @llvm.isnan.f64(double %x)
+  ret i1 %0
+}
+
+define i1 @isnan_ldouble(x86_fp80 %x) {
+; CHECK-32-LABEL: isnan_ldouble:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:fldt {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fxam
+; CHECK-32-NEXT:fstp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:andb $69, %ah
+; CHECK-32-NEXT:cmpb $1, %ah
+; CHECK-32-NEXT:sete %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_ldouble:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:fldt {{[0-9]+}}(%rsp)
+; CHECK-64-NEXT:fxam
+; CHECK-64-NEXT:fstp %st(0)
+; CHECK-64-NEXT:fnstsw %ax
+; CHECK-64-NEXT:andb $69, %ah
+; CHECK-64-NEXT:cmpb $1, %ah
+; CHECK-64-NEXT:sete %al
+; CHECK-64-NEXT:retq
+entry:
+  %0 = tail call i1 @llvm.isnan.f80(x86_fp80 %x)
+  ret i1 %0
+}
+
+define i1 @isnan_float_strictfp(float %x) strictfp {
+; CHECK-32-LABEL: isnan_float_strictfp:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:movl $2147483647, %eax # imm = 0x7FFF
+; CHECK-32-NEXT:andl {{[0-9]+}}(%esp), %eax
+; CHECK-32-NEXT:cmpl $2139095040, %eax # imm = 0x7F80
+; CHECK-32-NEXT:setne %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_float_strictfp:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff updated this revision to Diff 355614.
sepavloff added a comment.

Missed optimization in X86 codegen proposed by Craig


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/X86/strictfp_builtins.c
  clang/test/CodeGen/aarch64-strictfp-builtins.c
  clang/test/CodeGen/strictfp_builtins.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/ISDOpcodes.h
  llvm/include/llvm/CodeGen/TargetLowering.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Analysis/ConstantFolding.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
  llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
  llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
  llvm/lib/CodeGen/TargetLoweringBase.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/test/CodeGen/AArch64/aarch64-fpclass.ll
  llvm/test/CodeGen/PowerPC/ppc-fpclass.ll
  llvm/test/CodeGen/X86/x86-fpclass.ll
  llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll

Index: llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll
@@ -0,0 +1,28 @@
+; RUN: opt < %s -instsimplify -S | FileCheck %s
+
+define i1 @isnan_01() {
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float 0x7FF8)
+  ret i1 %0
+}
+; CHECK-LABEL: isnan_01
+; CHECK:   ret i1 true 
+
+define i1 @isnan_02() {
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float 0x7FF0)
+  ret i1 %0
+}
+; CHECK-LABEL: isnan_02
+; CHECK:   ret i1 false 
+
+define <4 x i1> @isnan_03() {
+entry:
+  %0 = tail call <4 x i1> @llvm.isnan.v4f32(<4 x float>)
+  ret <4 x i1> %0
+}
+; CHECK-LABEL: isnan_03
+; CHECK:   ret <4 x i1> 
+
+declare i1 @llvm.isnan.f32(float)
+declare <4 x i1> @llvm.isnan.v4f32(<4 x float>)
Index: llvm/test/CodeGen/X86/x86-fpclass.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/x86-fpclass.ll
@@ -0,0 +1,655 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=i686 | FileCheck %s -check-prefix=CHECK-32
+; RUN: llc < %s -mtriple=x86_64 | FileCheck %s -check-prefix=CHECK-64
+
+define i1 @isnan_float(float %x) {
+; CHECK-32-LABEL: isnan_float:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:flds {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fucomp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:# kill: def $ah killed $ah killed $ax
+; CHECK-32-NEXT:sahf
+; CHECK-32-NEXT:setp %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_float:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:ucomiss %xmm0, %xmm0
+; CHECK-64-NEXT:setp %al
+; CHECK-64-NEXT:retq
+; NOSSE-32-LABEL: isnan_float:
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float %x)
+  ret i1 %0
+}
+
+define i1 @isnan_double(double %x) {
+; CHECK-32-LABEL: isnan_double:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:fldl {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fucomp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:# kill: def $ah killed $ah killed $ax
+; CHECK-32-NEXT:sahf
+; CHECK-32-NEXT:setp %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_double:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:ucomisd %xmm0, %xmm0
+; CHECK-64-NEXT:setp %al
+; CHECK-64-NEXT:retq
+entry:
+  %0 = tail call i1 @llvm.isnan.f64(double %x)
+  ret i1 %0
+}
+
+define i1 @isnan_ldouble(x86_fp80 %x) {
+; CHECK-32-LABEL: isnan_ldouble:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:fldt {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fxam
+; CHECK-32-NEXT:fstp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:andb $69, %ah
+; CHECK-32-NEXT:cmpb $1, %ah
+; CHECK-32-NEXT:sete %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_ldouble:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:fldt {{[0-9]+}}(%rsp)
+; CHECK-64-NEXT:fxam
+; CHECK-64-NEXT:fstp %st(0)
+; CHECK-64-NEXT:fnstsw %ax
+; CHECK-64-NEXT:andb $69, %ah
+; CHECK-64-NEXT:cmpb $1, %ah
+; CHECK-64-NEXT:sete %al
+; CHECK-64-NEXT:retq
+entry:
+  %0 = tail call i1 @llvm.isnan.f80(x86_fp80 %x)
+  ret i1 %0
+}
+
+define i1 @isnan_float_strictfp(float %x) strictfp {
+; CHECK-32-LABEL: isnan_float_strictfp:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:movl $2147483647, %eax # imm = 0x7FFF
+; CHECK-32-NEXT:andl {{[0-9]+}}(%esp), %eax
+; CHECK-32-NEXT:cmpl $2139095040, %eax # imm = 0x7F80
+; CHECK-32-NEXT:setne %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_float_strictfp:
+; CHECK-64: 

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D104854#2841505 , @efriedma wrote:

> I'd like to see some test coverage for all the floating-point types (half, 
> bfloat16, ppc_fp128).

Tests for half are added to aarch64-fpclass.ll, new file was created to test 
ppc_fp128. As for bfloat16, tests are added to aarch64-fpclass.ll but only in 
strictfp mode. Without this attribute codegen creates unordered compare 
operation, which is not supported for bfloat16.




Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:662
+  EVT NewResultVT = TLI.getTypeToTransformTo(*DAG.getContext(), ResultVT);
+  return DAG.getNode(N->getOpcode(), DL, NewResultVT, N->getOperand(0));
+}

craig.topper wrote:
> Don't you net to preserve the NoFPExcept flag? Same with all the other type 
> legalization functions
Yes, it is more correct way. Updated functions.



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:600
+
+  SDValue Res = DAG.getNode(ISD::ISNAN, DL, ResultVT, Arg);
+  // Vectors may have a different boolean contents to scalars.  Promote the

craig.topper wrote:
> If this is ResultVT then the Extend created next is always a NOP. Should this 
> be MVT::i1?
Indeed. Thank you!



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4727
+
+  if (TLI.isTypeLegal(WideResultVT)) {
+SDValue WideNode = DAG.getNode(ISD::ISNAN, DL, WideResultVT, WideArg);

craig.topper wrote:
> I wonder if we should be using getSetCCResultType here like WidenVecOp_SETCC?
Rewritten the function in this way.



Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6371
+DAG.getNode(ISD::ISNAN, sdl, DestVT, getValue(I.getArgOperand(0)));
+V->setFlags(Flags);
+// If ISD::ISNAN should be expanded, do it right now, because the expansion

craig.topper wrote:
> Why not pass flags to getNode?
Yes, it should be set via getNode.



Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6375
+// types prior to selection.
+if (!TLI.isOperationLegalOrCustom(ISD::ISNAN, ArgVT)) {
+  SDValue Result = TLI.expandISNAN(V.getNode(), DAG);

craig.topper wrote:
> This breaks if we add constant folding for ISD::ISNAN to getNode.
Now expansion occurs before getNode.



Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6967
+  // NaN has all exp bits set and a non zero significand. Therefore:
+  // isnan(V) == ((exp mask - (abs(V) & exp mask)) < 0)
+  unsigned BitSize = OperandVT.getScalarSizeInBits();

thopre wrote:
> I seem to have made a mistake when I wrote this.
Thank you! I updated the comment.



Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6981
+  // V = sign bit (Sub) <=> V = (Sub < 0)
+  SDValue SignBitV = DAG.getNode(ISD::SRL, DL, IntVT, Sub,
+ DAG.getConstant(BitSize - 1, DL, IntVT));

craig.topper wrote:
> Why can't we just check < 0 here? Why do we need to shift?
It seems the shift is not needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff updated this revision to Diff 355539.
sepavloff added a comment.
Herald added subscribers: kbarton, nemanjai.

Addressed reviewer's notes

- Math flags are set when ISNAN node is transformed,
- They are set in calls to getNode,
- WidenVecOp_ISNAN is made similar to WidenVecOp_SETCC,
- Building ISNAN node was changed,
- Fixed error in comment,
- Removed extra shift.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/X86/strictfp_builtins.c
  clang/test/CodeGen/aarch64-strictfp-builtins.c
  clang/test/CodeGen/strictfp_builtins.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/ISDOpcodes.h
  llvm/include/llvm/CodeGen/TargetLowering.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Analysis/ConstantFolding.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
  llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
  llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
  llvm/lib/CodeGen/TargetLoweringBase.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/test/CodeGen/AArch64/aarch64-fpclass.ll
  llvm/test/CodeGen/PowerPC/ppc-fpclass.ll
  llvm/test/CodeGen/X86/x86-fpclass.ll
  llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll

Index: llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll
@@ -0,0 +1,28 @@
+; RUN: opt < %s -instsimplify -S | FileCheck %s
+
+define i1 @isnan_01() {
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float 0x7FF8)
+  ret i1 %0
+}
+; CHECK-LABEL: isnan_01
+; CHECK:   ret i1 true 
+
+define i1 @isnan_02() {
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float 0x7FF0)
+  ret i1 %0
+}
+; CHECK-LABEL: isnan_02
+; CHECK:   ret i1 false 
+
+define <4 x i1> @isnan_03() {
+entry:
+  %0 = tail call <4 x i1> @llvm.isnan.v4f32(<4 x float>)
+  ret <4 x i1> %0
+}
+; CHECK-LABEL: isnan_03
+; CHECK:   ret <4 x i1> 
+
+declare i1 @llvm.isnan.f32(float)
+declare <4 x i1> @llvm.isnan.v4f32(<4 x float>)
Index: llvm/test/CodeGen/X86/x86-fpclass.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/x86-fpclass.ll
@@ -0,0 +1,655 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=i686 | FileCheck %s -check-prefix=CHECK-32
+; RUN: llc < %s -mtriple=x86_64 | FileCheck %s -check-prefix=CHECK-64
+
+define i1 @isnan_float(float %x) {
+; CHECK-32-LABEL: isnan_float:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:flds {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fucomp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:# kill: def $ah killed $ah killed $ax
+; CHECK-32-NEXT:sahf
+; CHECK-32-NEXT:setp %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_float:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:ucomiss %xmm0, %xmm0
+; CHECK-64-NEXT:setp %al
+; CHECK-64-NEXT:retq
+; NOSSE-32-LABEL: isnan_float:
+entry:
+  %0 = tail call i1 @llvm.isnan.f32(float %x)
+  ret i1 %0
+}
+
+define i1 @isnan_double(double %x) {
+; CHECK-32-LABEL: isnan_double:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:fldl {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fucomp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:# kill: def $ah killed $ah killed $ax
+; CHECK-32-NEXT:sahf
+; CHECK-32-NEXT:setp %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_double:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:ucomisd %xmm0, %xmm0
+; CHECK-64-NEXT:setp %al
+; CHECK-64-NEXT:retq
+entry:
+  %0 = tail call i1 @llvm.isnan.f64(double %x)
+  ret i1 %0
+}
+
+define i1 @isnan_ldouble(x86_fp80 %x) {
+; CHECK-32-LABEL: isnan_ldouble:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:fldt {{[0-9]+}}(%esp)
+; CHECK-32-NEXT:fxam
+; CHECK-32-NEXT:fstp %st(0)
+; CHECK-32-NEXT:fnstsw %ax
+; CHECK-32-NEXT:andb $69, %ah
+; CHECK-32-NEXT:cmpb $1, %ah
+; CHECK-32-NEXT:sete %al
+; CHECK-32-NEXT:retl
+;
+; CHECK-64-LABEL: isnan_ldouble:
+; CHECK-64:   # %bb.0: # %entry
+; CHECK-64-NEXT:fldt {{[0-9]+}}(%rsp)
+; CHECK-64-NEXT:fxam
+; CHECK-64-NEXT:fstp %st(0)
+; CHECK-64-NEXT:fnstsw %ax
+; CHECK-64-NEXT:andb $69, %ah
+; CHECK-64-NEXT:cmpb $1, %ah
+; CHECK-64-NEXT:sete %al
+; CHECK-64-NEXT:retq
+entry:
+  %0 = tail call i1 @llvm.isnan.f80(x86_fp80 %x)
+  ret i1 %0
+}
+
+define i1 @isnan_float_strictfp(float %x) strictfp {
+; CHECK-32-LABEL: isnan_float_strictfp:
+; CHECK-32:   # %bb.0: # %entry
+; CHECK-32-NEXT:movl $2147483647, %eax 

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

A few of the AArch64 sequences don't look ideal, but that's not the fault of 
your patch.

I'd like to see some test coverage for all the floating-point types (half, 
bfloat16, ppc_fp128).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:22039
+  // Move FPSW to AX.
+  SDValue FPSW = DAG.getCopyToReg(DAG.getEntryNode(), DL, X86::FPSW, Test,
+  SDValue());

The code you copied this form was overly complicated. You can output Glue 
instead of MVT::i16 from XAM node and then pass that directly to FNSTSW16r in 
place of `FPSW, FPSW.getValue(1)`.  I have made this change to 
X86ISelDAGToDAG.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:662
+  EVT NewResultVT = TLI.getTypeToTransformTo(*DAG.getContext(), ResultVT);
+  return DAG.getNode(N->getOpcode(), DL, NewResultVT, N->getOperand(0));
+}

Don't you net to preserve the NoFPExcept flag? Same with all the other type 
legalization functions



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:600
+
+  SDValue Res = DAG.getNode(ISD::ISNAN, DL, ResultVT, Arg);
+  // Vectors may have a different boolean contents to scalars.  Promote the

If this is ResultVT then the Extend created next is always a NOP. Should this 
be MVT::i1?



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4727
+
+  if (TLI.isTypeLegal(WideResultVT)) {
+SDValue WideNode = DAG.getNode(ISD::ISNAN, DL, WideResultVT, WideArg);

I wonder if we should be using getSetCCResultType here like WidenVecOp_SETCC?



Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6371
+DAG.getNode(ISD::ISNAN, sdl, DestVT, getValue(I.getArgOperand(0)));
+V->setFlags(Flags);
+// If ISD::ISNAN should be expanded, do it right now, because the expansion

Why not pass flags to getNode?



Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6375
+// types prior to selection.
+if (!TLI.isOperationLegalOrCustom(ISD::ISNAN, ArgVT)) {
+  SDValue Result = TLI.expandISNAN(V.getNode(), DAG);

This breaks if we add constant folding for ISD::ISNAN to getNode.



Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6981
+  // V = sign bit (Sub) <=> V = (Sub < 0)
+  SDValue SignBitV = DAG.getNode(ISD::SRL, DL, IntVT, Sub,
+ DAG.getConstant(BitSize - 1, DL, IntVT));

Why can't we just check < 0 here? Why do we need to shift?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Thomas Preud'homme via Phabricator via cfe-commits]

thopre added a comment.

LGTM (besides comment fix) but I'm not too familiar with the vector side of 
things




Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6967
+  // NaN has all exp bits set and a non zero significand. Therefore:
+  // isnan(V) == ((exp mask - (abs(V) & exp mask)) < 0)
+  unsigned BitSize = OperandVT.getScalarSizeInBits();

I seem to have made a mistake when I wrote this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D104854#2838754 , @craig.topper 
wrote:

> Doesn't gcc also fold isnan to false under fast math? If we diverge here that 
> means your code would only work correctly with clang.

GCC does the same transformation, ICC and MSVC do not: 
https://godbolt.org/z/ovboWqPeb. Both clang and GCC do this transformation only 
with optimization level > 0. With -O0 both produce code that does real check, 
so semantic of the produced code is different depending on optimization level, 
it looks more like a bug rather than feature.

There is a GCC ticket: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84949, 
which refers to the similar thing. It is created against to libstdc++ but the 
reason is in the compiler. In one on the comments there 
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84949#c8) an opinion is expressed:

  ... My conclusion: std::numeric_limits means "has NaN bitpattern" and "has 
IEC559 bit layout" not "has NaNs with NaN behavior" and "has IEC559 behavior".

So this behavior is considered as incorrect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

Doesn't gcc also fold isnan to false under fast math? If we diverge here that 
means your code would only work correctly with clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D104854#2838659 , @jdoerfert wrote:

> In D104854#2838495 , @sepavloff 
> wrote:
>
>> In D104854#2838468 , @thopre wrote:
>>
>>> Are you planning to do this for the other FP test builtin (isinf, isfinite, 
>>> isinf_sign, isnormal)?
>>
>> Yes, they have similar problems. 
>> If someone would like to implement them it would be nice.
>
> From a user perspective, it seems sub-optimal to postpone the others "untile 
> someone wants to implement them".
> Once `isnan` behaves differently than the rest, I can see users being 
> confused and rightfully so.
> I don't have a strong opinion but I'd prefer we switch them over together.

Sure. I Just want to say that if someone wants or has plans to implement these 
functions, I appreciate these efforts. This functionality is in my plans. 
Considering only one function in this patch must facilitatу the review process.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D104854#2838495 , @sepavloff wrote:

> In D104854#2838468 , @thopre wrote:
>
>> Are you planning to do this for the other FP test builtin (isinf, isfinite, 
>> isinf_sign, isnormal)?
>
> Yes, they have similar problems. 
> If someone would like to implement them it would be nice.

From a user perspective, it seems sub-optimal to postpone the others "untile 
someone wants to implement them".
Once `isnan` behaves differently than the rest, I can see users being confused 
and rightfully so.
I don't have a strong opinion but I'd prefer we switch them over together.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D104854#2838468 , @thopre wrote:

> Are you planning to do this for the other FP test builtin (isinf, isfinite, 
> isinf_sign, isnormal)?

Yes, they have similar problems. 
If someone would like to implement them it would be nice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Thomas Preud'homme via Phabricator via cfe-commits]

thopre added a comment.

Are you planning to do this for the other FP test builtin (isinf, isfinite, 
isinf_sign, isnormal)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854

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

[PATCH] D104854: Introduce intrinsic llvm.isnan

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff created this revision.
sepavloff added reviewers: efriedma, kpn, thopre, jonpa, cameron.mcinally, 
RKSimon, craig.topper.
Herald added subscribers: jdoerfert, pengfei, hiraditya.
sepavloff requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.

Clang has builtin function '__builtin_isnan', which implements C
library function 'isnan'. This function now is implemented entirely in
clang codegen, which expands the function into set of IR operations.
There are three mechanisms by which the expansion can be made.

- The most common mechanism is using an unordered comparison made by 
instruction 'fcmp uno'. This simple solution is target-independent and works 
well in most cases. It however is not suitable if floating point exceptions are 
tracked. Corresponding IEEE 754 operation and C function must never raise FP 
exception, even if the argument is a signaling NaN. Compare instructions 
usually does not have such property, they raise 'invalid' exception in such 
case. So this mechanism is unsuitable when exception behavior is strict. In 
particular it could result in unexpected trapping if argument is SNaN.

- Another solution was implemented in https://reviews.llvm.org/D95948. It is 
used in the cases when raising FP exceptions by 'isnan' is not allowed. This 
solution implements 'isnan' using integer operations. It solves the problem of 
exceptions, but offers one solution for all targets, however some can do the 
check in more efficient way.

- Solution implemented by https://reviews.llvm.org/D96568 introduced a hook 
'clang::TargetCodeGenInfo::testFPKind', which injects target specific code into 
IR. Now only SystemZ implements this hook and it generates a call to target 
specific intrinsic function.

Although these mechanisms allow to implement 'isnan' with enough
efficiency, expanding 'isnan' in clang has drawbacks:

- The operation 'isnan' is hidden behind generic integer operations or 
target-specific intrinsics. It complicates analysis and can prevent some 
optimizations.

- IR can be created by tools other than clang, in this case treatment of 
'isnan' has to be duplicated in that tool.

Another issue with the current implementation of 'isnan' comes from the
use of options '-ffast-math' or '-fno-honor-nans'. If such option is
specified, 'fcmp uno' may be optimized to 'false'. It is valid
optimization in general, but it results in 'isnan' always returning
'false'. For example, in some libc++ implementations the following code
returns 'false':

  std::isnan(std::numeric_limits::quiet_NaN())

The options '-ffast-math' and '-fno-honor-nans' imply that FP operation
operands are never NaNs. This assumption however should not be applied
to the functions that check FP number properties, including 'isnan'. If
such function returns expected result instead of actually making
checks, it becomes useless in many cases. The option '-ffast-math' is
often used for performance critical code, as it can speed up execution
by the expense of manual treatment of corner cases. If 'isnan' returns
assumed result, a user cannot use it in the manual treatment of NaNs
and has to invent replacements, like making the check using integer
operations. There is a discussion in https://reviews.llvm.org/D18513#387418,
which also expresses the opinion, that limitations imposed by
'-ffast-math' should be applied only to 'math' functions but not to
'tests'.

To overcome these drawbacks, this change introduces a new IR intrinsic
function 'llvm.isnan', which realises the check as specified by IEEE-754
and C standards in target-agnostic way. During IR transformations it
does not undergo undesirable optimizations. It reaches instruction
selection, where is lowered in target-dependent way. The lowering can
vary depending on options like '-ffast-math' or '-ffp-model' so the
resulting code satisfies requested semantics.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104854

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/X86/strictfp_builtins.c
  clang/test/CodeGen/aarch64-strictfp-builtins.c
  clang/test/CodeGen/strictfp_builtins.c
  llvm/docs/LangRef.rst
  llvm/include/llvm/CodeGen/ISDOpcodes.h
  llvm/include/llvm/CodeGen/TargetLowering.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Analysis/ConstantFolding.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
  llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
  llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
  llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
  llvm/lib/CodeGen/TargetLoweringBase.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/test/CodeGen/AArch64/aarch64-fpclass.ll
  llvm/test/CodeGen/X86/x86-fpclass.ll
  llvm/test/Transforms/InstSimplify/ConstProp/fpclassify.ll

Index: