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.

Rebase.

Update comment as requested.

On targets that support static rounding mode (like RISCV) dynamic and constant rounding modes may be different and the behavior changes if default mode is replaced by dynamic.

Whether a target supports static rounding modes on floating-point instructions is completely irrelevant. The user-visible behavior must be the same either way. If a target doesn't have specialized instructions, the code generator can save/restore the rounding mode. This should be transparent to the user; the user can't read the rounding mode in between the save and restore. (We already do this sort of rounding mode manipulation on x86, to implement float-to-int conversion on targets without SSE.)

Update for review comments: eliminate unneeded checks.

I've verified that it fixes the build on FreeBSD 11.1 and the current version of FreeBSD 12.

Will this fix FreeBSD 12 (and maybe 11?). FreeBSD 12 is still very much supported.

Ping.

Update for review comments.

Update for review comments.

I realized that going through isInstructionTriviallyDead() means it will handle constrained intrinsics differently than it does now. So this isn't strictly speaking an NFC change.

Not needed anymore after D69798 landed.

I trust you on the instruction set changes. LGTM.

Rebase.

In D69798#3408468, @andrew.w.kaylor wrote:

I had forgotten that this patch never landed, but I was investigating a bug yesterday that I think this will help with (https://github.com/llvm/llvm-project/issues/48669).

@kpn are you happy with the current form. It's gotten stale in a few places, but I think it's basically correct.

Rebase. Ping.

Ping.

In D118259#3389246, @fhahn wrote:

In D118259#3389235, @kpn wrote:

It's been a while, but I think the aarch64-neon-intrinsics-constrained.c test is trimmed down from the aarch64-neon-intrinsics.c test. Shouldn't the constrained and non-constrained end-to-end tests be treated the same?

Are you referring to https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGen/aarch64-neon-intrinsics.c? This one doesn't check assembly.

It's been a while, but I think the aarch64-neon-intrinsics-constrained.c test is trimmed down from the aarch64-neon-intrinsics.c test. Shouldn't the constrained and non-constrained end-to-end tests be treated the same?

In D118426#3337963, @sepavloff wrote:

In D118426#3337489, @kpn wrote:

It seems like these dangling, useless instructions would interfere with subsequent optimizations until finally they might or might not get removed here. And constant folding isn't sufficient to duplicate the logic of the transform pass that determined the instruction could be deleted.

InstCombine is called several times, it calls constant folding and instruction simplification functions. It looks like a good place to get rid of such useless instructions.

I'm still not clear on why, when a transform pass knows an instruction can be deleted, we are required to keep that instruction around anyway. It seems like these dangling, useless instructions would interfere with subsequent optimizations until finally they might or might not get removed here. And constant folding isn't sufficient to duplicate the logic of the transform pass that determined the instruction could be deleted.

Please wait a couple of days in case there are any comments from anyone else.

In D64746#3325655, @nlopes wrote:

In D64746#3325599, @uweigand wrote:

In D64746#3324418, @nlopes wrote:

In D64746#3324270, @uweigand wrote:

This matches my understanding. (Though I must admit I'm not 100% confident I fully understand the precise distinction between "undefined behavior", a poison value, and an undef value ... It's certainly one of those :-) But from my reading of the IR reference, "poison" does indeed look correct here.)

OK, thank you!
It's poison. UB would be too strong. I've put on my todo list to fix LangRef as well.

The reason why I was hesitating about poison vs. UB is exceptions. This is clearer when talking about the exception metadata as opposed to the rounding mode metadata: if exception metadata promises that FP exceptions are disabled, but they are actually enabled at runtime, compiler optimizations are free to generate code triggering spurious exceptions that might kill the program outright - that seems more like UB than poison to me.

With the rounding mode, this is much less obvious to me, but I guess in theory it could happen that if the actual rounding mode does not match the mode promised by the metadata, a compiler optimization might possibly generate code that could now e.g. introduce an underflow where the unoptimized code would not have one, and if in addition exception are also enabled, that extra underflow could now trigger an extra exception. However, I'm not sure if that can actually ever occur in practice ...

Thank you!
Makes sense. I need to think a bit more about this.
Ideally we want to allow intrinsics marked as not throwing exceptions to be executed speculatively. The semantics needs to be crafted to allow that scenario.
I think returning poison for the rounding mode is OK as that is sufficient to trigger an exception (as poison can be replaced with any value). So we can keep the mismatch in rounding modes as poison. And then move the complexity to the exception semantics. It's not obvious to me what's the ideal semantics yet.

Remove unneeded includes, and correct the placement of the include of FMF.h.

In D111437#3100240, @Kai wrote:

More comments about the implementation?
E.g. is the name flags_t acceptable?

Update for review comments: don't fold away a case that can and should result in -0.0. Add tests for this very case.

LGTM

No, actually wouldInstructionBeTriviallyDead() rejects some instructions that the previous isSafeToRemove() function allowed. By trusting a true returned by wouldInstructionBeTriviallyDead(), but using the old logic if it returned false, we can make this change NFC. To that end I have also removed the constrained FP logic and I plan on adding it back in probably D115737.

In D118319#3275319, @nikic wrote:

I think it would be better to do this as part of InstCombine. I don't think having a single pass at the end of the pipeline would be a good solution to this, as it means that these dead calls will survive through most of the optimization pipeline, blocking cost models and other optimizations. This doesn't look expensive enough that including it in InstCombine would be a problem.

In IRBuilder.h we have this:

In D115804#3202479, @sepavloff wrote:

In D115804#3201681, @spatel wrote:

In D115804#3201044, @craig.topper wrote:

What's the plan for constrained intrinsics versions of these intrinsics? The IRBuilder calls for CreateFPToSI and CreateFPToUI are strict FP aware, but this new code isn't.

Not sure. cc'ing @kpn @sepavloff @andrew.w.kaylor
The saturating intrinsics implement non-standard behavior for C languages AFAIK, so we might want to warn if someone tries to use "-fno-strict-float-cast-overflow" and "-ffp-exception-behavior=strict" at the same time? Or we try to support that corner case by adding even more FP intrinsics?

Conversion float->int depends on rounding mode. At least on some ML cores this conversion is made with saturating semantics. So ability to specify rounding mode would be useful for such targets.

My guess:

I don't see how this will resolve my issue. I'll start a new thread on llvm-dev. Don't let me block this review.

BTW, for a future ticket: having curl enabled without first checking to see if it is installed creates builds failures on non-Linux hosts. OK, yes, it can be disabled with a cmake argument, but it would be better if the extra cmake argument wasn't needed. A warning at cmake configure time that says what features will be missing without curl is probably a good idea as well.

That flag tells front ends that strictfp will generate correct instructions. Don't set it if more work is needed for correct instructions. The support should at least have parity with X86 and SystemZ.

In D114766#3161426, @nikic wrote:

Marking as changes requested per above comment -- really don't think this is the right place to address this, since it will just create an incorrect picture of what LLVM can optimize in practice. Usually, this fold will happen via InstCombine rather than InstSimplifyPass. But I don't think changing InstCombine is the right solution either, as any number of passes do this kind of "simplify and DCE" pattern.

In D114766#3161806, @sepavloff wrote:

Probably the pass should run earlier, it is hard to reason without analyzing practical cases. If it is indeed so, we could move the pass or run it several times. Tuning pipeline is not unique for constant folding only. But without ability to remove instructions many optimizations do not make sense.

In D114766#3161749, @lebedev.ri wrote:

In D114766#3161737, @sepavloff wrote:

In D114766#3161688, @kpn wrote:

In D114766#3161680, @sepavloff wrote:

Moving this logic to wouldInstructionBeTriviallyDead() does not seem right, because the decision about removing side effect comes from constant evaluation and not from the state of call object.

I'm sympathetic to this argument. If a transformation pass determines that a constrained intrinsic can be removed then it would be helpful if that pass could note that fact. This doesn't help us with analysis passes, though. And I'm a little worried about having to explicitly mark constrained intrinsics as removable since I don't know that we do that for any other instruction. If constrained intrinsics are the only time we do this then it seems error prone. Maybe I'm wrong?

The ability to remove side effect is vital for the implementation of floating point arithmetic. There are several possible optimization techniques, which depend on this possibility. For example, if FP operations in a function have attributes "round.dynamic" and "fpexcept.ignore" and we know that rounding mode is not changed in this function, we could remove side effect of all the operations, which can give performance of non-constrained operations. As we cannot mix ordinary and constrained operation in the same function, the only way is to control side effect.

We aren't saying that the ocean isn't full of water, we are only saying that you are looking at the pond while thinking it's an ocean.
We are talking about different things here. Fixing InstSimplify pass will not do any good, because it's effectively not used.

In D114766#3161680, @sepavloff wrote:

Moving this logic to wouldInstructionBeTriviallyDead() does not seem right, because the decision about removing side effect comes from constant evaluation and not from the state of call object.

Well, the problem is more widespread than that. IPSCCP can create instructions where it knows the result, and it knows the instruction will never trap. Consider how it can propagate two constants into a compare instruction. It knows the values are constants, it knows they are not undef, poison, NaN, Inf, it knows the value of the compare, and it uses that knowledge to further propagate constants and simplify the code. But it can't remove the constrained compare instruction because we don't have a good way to tell it the instruction has no side effects. It does call Instruction::isSafeToRemove(), and it does _not_ call wouldInstructionBeTriviallyDead(). A more centralized approach would be helpful.

Replaced with D112256. This patch here has restrictions that are not needed. To avoid confusing reading I'm replacing the review.

Everything that can be tested is now pushed in other tickets. I'll keep this around as a reminder to come back when m_FSub() is updated, but it doesn't need any action from anyone right now.

OK, this is overkill per Andy's comment. I'll update.

As requested, add tests for rounding to negative infinity, exceptions ignored, no sized zeros fast math flag.

Update for review comments.

As for llvm langref, I think the following statement is cited here: https://llvm.org/docs/LangRef.html#floating-point-environment:

The default LLVM floating-point environment assumes that floating-point instructions do not have side effects. Results assume the round-to-nearest rounding mode. No floating-point exception state is maintained in this environment. Therefore, there is no attempt to create or preserve invalid operation (SNaN) or division-by-zero exceptions.

This sentence is about exceptions but not the results of operations. So actually nothing in the langref justifies this transformation.

The wording could be made more explicit, but as I suggested in my comment from Aug 6 in this review: the default LLVM FP env ignores exceptions, so citing the IEEE-754 clause that begins with "under default exception handling" to describe SNAN->QNAN behavior is not applicable.

This is peculiarities of IEEE-754 language. Exception handling in this context is what to do if exceptional situation occurs. In 7.1 there is a description of the default exception handling:

This clause also specifies default non-stop exception handling for exception signals, which is to deliver a
default result, continue execution, and raise the corresponding status flag (except in the case of exact
underflow, see 7.5).

This is exactly the way the exceptions are handled in the default FP environment. Exception must be handled in some way because hardware always signals them if exceptional situation occurs.

In D106362#3031030, @sepavloff wrote:

In D106362#3030871, @kpn wrote:

In D106362#3030856, @sepavloff wrote:

In D106362#3030790, @kpn wrote:

Changing the way the existing transforms check for FMF.noNaNs() sounds like a different ticket that needs to be done before this one can progress. Unless I misunderstood?

I know nothing about such change.

Your proposed changes around SNaN will affect the non-constrained cases.

I think the optimization fadd X, -0 ==> X in general case (not FMF.noNaNs()) is incorrect. The obtained values must be identical to what would produce hardware, otherwise it is not an optimization. If you think that such change deserves a separate patch, no problem. It seems to me that this patch could establish correct folding even if it changes non-constrained operation as well.