Would a constrained version of this be needed?

There's a lot of code here. I'll try to look at it in the next couple of weeks. I was out last week and I'm a little behind.

Why delete from class ENDRecord the support for having the entry point specified on the END card? Wouldn't that be useful in a dumping tool at least? Or is this handled somewhere else now?

The constrained intrinsic version is not documented here.

LGTM

LGTM as well.

Are we certain that a constrained sqrt() folded away will be as accurate as not folded when the rounding mode is different? There's a lot of permutations of library calls and instructions that constrained sqrt() can be lowered to, and I'm worried that the results will be surprisingly bad with different rounding modes. If we don't fold then the end user can deal with library issues themselves at least, and hopefully square root instructions will be on CPUs that properly handle the different rounding modes.

Seems reasonable. LGTM.

LGTM "for reals" this time.

In D148138#4265164, @arsenm wrote:

Though I am wondering if there's any real reason to require anything of an indirect call site

LGTM

Once the prereqs are in place this LGTM.

Verify that CallBase::isStrictFP() continues to do what we think it is doing.

Update for review comments.

I don't see any tests for a constrained/STRICT tan. We do have tests for constrained sin and cos so tan tests where sin and cos are tested is probably appropriate.

Where is fpclassTestIsFCmp0 implemented?

This is no longer needed as the pieces have all made it into the tree already.

LGTM.

What's the plan for tying this to strictfp? Because I don't it should be tied to cases where we use the constrained intrinsics but the exceptions are ignored and the default rounding is in stated. Those instructions are supposed to behave the same as the non-constrained instructions. So keying off the presence of the strictfp attribute on the function definition, or the (equivalent) presence of constrained intrinsics, would be too simple.

In D143074#4114288, @Muon wrote:

Hold on, that still says that operations assume that all NaNs are quiet. Doesn't that mean that passing a signaling NaN to an operation is potentially undefined behavior? Can we instead say that math operations treat all NaNs as if they were quiet NaNs? Does that run into issues with pow() and friends?

In D143074#4114952, @programmerjake wrote:

In D143074#4114910, @RalfJung wrote:

Then, LLVM is broken on old MIPS. There's just no way it's okay to spuriously introduce sNaNs when the original program didn't contain sNaNs in the first place. It results in incorrect results, without the original user code breaking any assumptions.

I think I am trying to understand why this is the case.

because, from what I understand, before IEEE 754 specified how to encode quietness for NaNs, MIPS (and PA-RISC) arbitrarily chose the opposite encoding to what IEEE 754-2008 specifies, so LLVM generating quiet NaNs following IEEE 754-2008 produces NaNs that are actually signalling NaNs for old MIPS. MIPS later added a mode bit allowing swapping its interpretation of signalling/quiet NaNs to fall in line with the IEEE 754-2008 spec -- new MIPS has that set to IEEE 754-2008 mode.

So the alternative is to say that if any input is an sNaN, then it may be treated as if it was a qNaN and output NaN have arbitrary quietness, but if there are no sNaN inputs then all output NaN will be quiet.

That works, except for old MIPS, where LLVM's idea of what's quiet/signalling doesn't currently match.

I think that about covers it. LGTM.

In D143074#4112184, @Muon wrote:

In D143074#4110459, @spatel wrote:

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.

Although I am uncertain about the consequences of changing LLVM's defaults, I would like to point out that there may not actually be a performance impact to handling signaling NaNs correctly. Replacing x * 1.0 (and the like) with x is sound whenever the only uses of x are in quieting floating-point operations (that is, +, -, *, /, fma, rem, sqrt, but not bitwise ops such as negate, abs or copySign). It would pretty much only affect cases where someone was just performing an isolated multiplication by 1 and returning that. (Such as, say, someone trying to quiet a possible-signaling value.)

In D143074#4106277, @sepavloff wrote:

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

We use "dynamic" for the constrained intrinsics. I'd stay consistent with our terminology and stick with "dynamic" here.

I like the new wording. Taking the list of exceptions out was also a good idea.

In D137811#4081771, @sepavloff wrote:

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.

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.

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

In D137811#4063200, @kpn wrote:

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.

I didn't think we had code in the tree to convert normal FP instructions into constrained intrinsics. Andy Kaylor had a ticket with code to do this, but I didn't think it ever went in. Where is this code used by the inliner?

Here it is: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/CloneFunction.cpp#L390

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.

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

Assuming we don't have to worry about the new, 8 and 16 bit FP formats I'm happy to see this move forward.

It looks like clang does have at least one test that checks for the strictfp attribute on function definitions. We also have a number that test for the strictfp attribute on function calls. So I think our test coverage is in good shape.

Are we certain that a value that is very close to infinity won't trip over into infinity after rounding?

I believe this is correct. I'm looking at n3054 Annex F and it says that sqrt(+inf) returns +inf. But since you are checking that the input cannot be inf it follows that the output cannot be inf.

LGTM

LGTM

Abandoned in favor of D136466.

Update for review comments: allow folding for more than constrained intrinsics.

In D136097#3863230, @spatel wrote:

Ah, thanks for clearing that up. The "nsz" wording has always been a less solid than I'd like, but I'm not sure how to change it.
I suspect this transform would cause some fallout with users because it's a stretch to go from -0.0 isn't different than 0.0, therefore, -Inf isn't different than +Inf. There was a suggestion in a previous discussion that we really need entirely new FP types to use with fast-math-flags, so it's clear that -0.0, Inf, or NaN values simply do not exist when the corresponding compile flag is used, but I don't think anyone has looked seriously at implementing that. There's more discussion about the general problem here:
https://github.com/llvm/llvm-project/issues/51601

In D136097#3862946, @spatel wrote:

In D136097#3862928, @kpn wrote:

It looks like by 754 sec 7.3 a (-x)/(+0) == -Inf, but the nsz is documented as saying the sign of the result is "insignificant", so I think we're good here.

LGTM

"nsz" means the sign of a zero result is insignificant, not any result. -42.0 / 0.0 should be -Inf, not +Inf?

Precommit tests?

It looks like by 754 sec 7.3 a (-x)/(+0) == -Inf, but the nsz is documented as saying the sign of the result is "insignificant", so I think we're good here.

Looks like IEEE 754-2019 section 7.3 says this should result in Infinity, but the ninf flag excludes Infinity, so poison seems correct.

Update to use more targeted tests.

In D115737#3615950, @nikic wrote:

Why isn't this already handled by ConstantFoldCall?

D118387 was my ticket, and it was handled that way to preserve the existing behavior. I don't believe a wrapper is needed in this case.

Update for review comment: just use wouldInstructionBeTriviallyDead().

I was thinking that adding a test for strict exception behavior might make it look intentional that this true/false propagation isn't happening. But a test case with a note is perfectly fine.

Address review comments.

I'll get back to you on the tfpropagation.ll question. The test precommit:

I'll update the patch later today with that missing assertion.

Update diff after precommit of tests.

You know, I now believe that I can simplify this down to just this. The complication to treat constrained intrinsics as a memory update was put in when I added the restrictions on doing CSE across function calls. But since that restriction isn't needed I don't see the need for the rest of the complications. I don't think we need to go to heroics just for exception handlers, at least not at this time. This code is now much simpler.

Ping

Move more of Instruction::isSafeToRemove() into this function. The remainder of isSafeToRemove() isn't needed because we filter out function calls at the top of the changed function.

Ping

Ping

Update for review comments. I've also reduced it down to just fcmp and fcmps having a functional impact. I don't have a good way to test the binary operators just yet.