I am not proficient in Power ISA and rely on the results of testing mentioned in the patch summary.

What is the advantage of such lowering?

Could you please rebase the patch? Some changes for FPClassTest are already committed.

LGTM.

In D144454#4163688, @rjmccall wrote:

I see. If we're going to take the target-independent values specified by FLT_ROUNDS, then the original builtin name is more appropriate. Of course, this has the disadvantage of not allowing target-specific values that might exist beyond those specified in the standard; are we pretty certain that's not a problem in practice?

Working on x86, ARM, and AArch64 is great, but I'm a little worried about adding another builtin that works only on major targets and probably crashes on others. I suppose we've got some number of those already, though.

C standard function fesetround accepts rounding mode in the form of target-specific values like FE_TONEAREST. They usually represent corresponding bits in control register, so are different for different targets. llvm.set_rounding in contrast accepts rounding mode as target-independent value, the encoding is same as used in FLT_ROUNDS.

Thanks!

Thanks!

Thanks!

Remove copying StrictFPAttr from InstantiateAttrs

In D143919#4130178, @efriedma wrote:

If the "strictfp" attribute is based on the contents of the function body, should we recompute it when the function is instantiated, as opposed to copying it? For example, say you have a pragma inside an "if constexpr"; should that set the strictfp attribute if it's discarded?

Otherwise, I guess updateAttrsForLateParsedTemplate makes sense.

In D143919#4123922, @rjmccall wrote:

In D143919#4123712, @sepavloff wrote:

In D143919#4123616, @efriedma wrote:

We have code somewhere to generically copy attributes from function templates to instantiations, right? Why do we need to explicitly copy StrictFPAttr in particular, separate from that?

Could you please point me out to this code? I didn't find it.

It's the call to InstantiateAttrs in InitFunctionInstantiation. InstantiateAttrs hard-codes a list of attributes to clone, which seems like a bad design, but it's easy enough to add your attribute as another case to that list.

Changed the way to copy strictfp attribute

In D143919#4123616, @efriedma wrote:

We have code somewhere to generically copy attributes from function templates to instantiations, right? Why do we need to explicitly copy StrictFPAttr in particular, separate from that?

In D143074#4110459, @spatel wrote:

In D143074#4108919, @sepavloff wrote:

In D143074#4108036, @jyknight wrote:

The property of SNaN like SNan + 0.0 -> QNaN is not related to exceptions and must be preserved in default environment also.

Neither GCC nor Clang have considered sNaN-related semantics important to provide by default thus far, and I don't think we ought to start now, either.

Agreed - we're not changing the default LLVM behavior with this patch, and I have not heard any reasons why we should. IEEE-754-compliant SNaN handling isn't important enough to the majority of FP users to justify FP performance regressions.

In D143074#4108036, @jyknight wrote:

The property of SNaN like SNan + 0.0 -> QNaN is not related to exceptions and must be preserved in default environment also.

Neither GCC nor Clang have considered sNaN-related semantics important to provide by default thus far, and I don't think we ought to start now, either. We should continue to support them on an opt-in basis -- at least as far as the end-to-end behavior goes for Clang users. (If there's some good reason to do so, it'd be fine to contemplate changing the IR-level defaults.)

The property of SNaN like SNan + 0.0 -> QNaN is not related to exceptions and must be preserved in default environment also.

The alternative solution in D136354 was implemented.

Thanks!

In D137811#4095037, @jyknight wrote:

In D137811#4090256, @sepavloff wrote:

The C classification macros fpclassify, iscanonical, isfinite, isinf, isnan, isnormal,
issignaling, issubnormal, iszero, and signbit 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.

and this property must be kept no matter if exceptions are ignored or not.

No, the property does not need to be maintained if the exception flag state cannot be observed by correct code. And that's exactly the case at hand in FENV_ACCESS OFF code (and, correspondingly, non-strictfp in LLVM IR). In such code, you are not allowed to have trap-on-exception enabled, nor are you allowed to access the floating-point exception status flags, and, the state of the status flags is unspecified upon transitioning back to FENV_ACCESS ON code. Thus, within such code, we may set the status flags to any arbitrary value we wish -- nobody can (correctly) tell the difference.

LGTM.

Thanks!

In D137811#4085644, @arsenm wrote:

In D137811#4084663, @sepavloff wrote:

What about such code?

int get_code(float x) {
  return isnan(x) ? 1 : 2;
}
void func1(float x) {
  int code1 = get_code(x);
  ...
}
#pragma STDC FENV_ACCESS ON
void func2(float x) {
  int code1 = get_code(x);
  ...
}

Acoording to C standard it must work as intended. If compiler replaces isnan with comparison, the code would be broken.

This code didn't actually manipulate the floating point mode, just made it legal to do so for the scope of the pragma. If you were to insert code to modify the mode to enable FP exceptions inside func2, it would be illegal because you did not restore the mode before calling into code outside of the pragma scope. FENV_ACCESS doesn't require the compiler to fixup the floating point mode for you before calling into other code.

In D137811#4082385, @arsenm wrote:

On most targets turning off FP exceptions means reading content of FP control register, changing value of mask bits and putting the modified register value back. It is expensive operation and cannot be made invisible to LLVM. Some targets (like RISCV) do not have possibility to mask FP exceptions at all, so at IR level there is no way to turn exception off. Default FP environment supposes that the exceptions are ignored, not disabled. In general case they are raised always.

Managing the FP mode is a user responsibility. If the operation in unconstrained we can assume the default floating point environment without preserved exceptions.

LGTM.

In D137811#4068836, @kpn wrote:

In D137811#4066479, @arsenm wrote:

In D137811#4065720, @arsenm wrote:

But they have different behavior if x is signaling NaN. Although the replacement is proposed for strictfp functions only, inlining may require conversion to a form that uses constrained intrinsics. The resulting code would have different behavior than original.

A transformation that’s valid for !strictfp functions cannot be invalid because of the potential for being inlined into a strictfp function. If this isn’t possible, there is a representational issue the inliner would need to account for.

Thinking about this more, I think any situation where this could be an issue would be malformed to begin with. You had a piece of code that was expecting exceptions to be enabled calling into code where exceptions were disabled without turning exceptions off

If exceptions were turned off, the call was made, and exceptions were turned back on then there would be no correctness issue. Inlining the called function wouldn't change that. The toggling of the exception enablement would be invisible to LLVM. Thus we can't categorically say that a strictfp function calling a !strictfp function is malformed.

Reverse ping.

Thanks!

Use --implicit-check-not in test

Thanks!

Use --implicit-check-not in tests

In D137811#4063159, @efriedma wrote:

inlining may require conversion to a form that uses constrained intrinsics

Functions which are not strictfp are allowed to introduce "spurious" fp flag writes; for example, we can flatten control flow that contains floating-point ops. Inlining the function doesn't change that general rule. The inliner converts fp operations just to ensure that later optimizations don't move those operations around.

In D141765#4059214, @kpn wrote:

Are we testing _Complex multiply or subtraction anywhere? I have a vague memory of multiply not working correctly.

Updated comment in test as proposed by Kevin

LGTM.

Existing decoration with nnan/ninf has inconvenient semantics. Having more powerfull and consistent solution can enable optimizations that are based on the knowledge of finitness of FP operands.

Early conversion is_fpclass->fcmp may result in incorrect semantics. It make the code:

if (!isnan(x)) {
…
}

indistinguishable from:

if (x == x) {
…
}

But they have different behavior if x is signaling NaN. Although the replacement is proposed for strictfp functions only, inlining may require conversion to a form that uses constrained intrinsics. The resulting code would have different behavior than original.

Remove support of __builtin_is{subnormal,zero,signaling}, rebased

Update patch

As mentioned in another review, using fcmp to represent isnormal in IR as well as any other classification function would result in incorrect code in strict mode. The source:

inline __attribute__((always_inline)) _Bool func(float x) {
  return __builtin_isnormal(x);
}

LGTM.

This change can have negative consequences in some cases. Some targets have dedicated instruction to test FP class and often this instruction is faster than arithmetic operations. Replacement of one operation with two arithmetic and two logic plus cost of FP constant materialization does not provide any visible benefit.

Remove __builtin_isfpclass

LGTM.

LGTM.

In D139316#3995455, @arsenm wrote:

Special case ppcfp128. Not sure if we need to exclude bfloat16 since it's exponent bits is larger than mantissa

Some mechanism must exists that applies this transformation or not depending on target. If instruction FCLASS is available, the transformation is profitable. But on targets that do not have such instruction, the replacement may produce inefficient code.

In D139316#3985122, @jcranmer-intel wrote:

In D139316#3984954, @sepavloff wrote:

PPC uses double double encoding (https://en.wikipedia.org/wiki/Quadruple-precision_floating-point_format), in which a number is represented by a sum of two double precision numbers, where the second just keeps additional mantissa bits. Total number of mantissa bits in ppc_fp128 is about 107 bits. Exponent range is the same as for double numbers: 2^-1022 to 2^1023. So large numbers (above 2^108) are always integers, they do not need rounding and the result of ceil, floor etc is the argument.

My understanding here is that the number of mantissa bits is effectively dynamic, since you could combine a large exponent (e.g., 2^1023) in the first number and a tiny exponent (e.g., 2^-50) in the second one, and all the mantissa bits in the middle would be implied 0. But I have no actual practical experience with ppc_fp128, and documentation for these kinds of things isn't great.

In D139859#3989536, @dblaikie wrote:

Does unix addr2line support this, or is there other precedent we should consider when evaluating this new feature?

Replace llvm::Optional with std::optional