In D61675#1642206, @mcberg2017 wrote:

Given that we may still want the binary fneg generated in Reassociate.cpp, this may be enough then, plus what was done in D66612.

In D66612#1641702, @mcberg2017 wrote:

BTW, general comment: Many of the remaining BinaryOperator contexts in Reassociate.cpp will need re-examaination too as UnaryOperator may also be present in places where it currently is not.

@mcberg2017 Do you have a test you can share?

Updated patch to generate unary FNeg in Clang.

LGTM. Less invasive is good.

Updated patch for Joe's suggestion.

Correct hash_combine(...) use.

Ah, I misunderstood hash_combine(...). This patch needs work...

Remove unnecessary {}'s.

In D61675#1609078, @cameron.mcinally wrote:

Kind of a larger question: Do we want to include the VisitUnaryMinus change into one mega-Diff? Or should we have several separate Diffs for each of the individual CreateFNeg(...) changes?

Thanks for pointing this out, Craig. I do see VisitUnaryMinus(...) now. [I'm glad that Clang distinguished between the unary and binary fneg -- that could've been trouble. ;)]

In D61675#1608593, @craig.topper wrote:

Sorry I wasn't very clear there. There are certainly places that use it, but the code for unary minus operator does not use CreateFNEG. it asks for the constant value to use for negation and the makes an fsub or sub depending on fp or int.

In D61675#1601893, @craig.topper wrote:

Clang does not use the IRBuilder fneg entry point. When do we plan to switch clang over?

How about we add a TODO comment regarding Cameron's topic? Would that be sufficient?

Saw that this was Accepted while I was writing my last comment...

2. The reassociation tests show that we were missing an optimization opportunity to fold away fneg-of-fneg. My reading of IEEE-754 says that all of these transforms are allowed (regardless of binop/unop fneg version) because:
   
   "For all other operations [besides copy/abs/negate/copysign], this standard does not specify the sign bit of a NaN result."
   
   In all of these transforms, we always have some other binop (fadd/fsub/fmul/fdiv), so we are free to flip the sign bit of a potential intermediate NaN operand. (If that interpretation is wrong, then we must already have a bug in the existing transforms?)

I don't see a problem with scalarizing strict ops for Custom lowered nodes that would otherwise be legal. It's not ideal, I suppose, but if a target doesn't support the strict nodes in the backend, then it probably shouldn't be using the experimental intrinsics anyway.

In D65226#1600831, @uweigand wrote:

In D65226#1600009, @cameron.mcinally wrote:

In D65226#1599743, @kpn wrote:

It looks like we're unrolling some vectors where before we weren't. That seems unfortunate. Is that the reason for the generated code quality regressions?

+1. What's the reason behind the scalarization?

Those are cases where the non-strict operation actually is not legal: e.g. the X86 target sets the operation action for FADD and FSUB vector operations to Custom. The old code simply ignored that and just emitted FADD and FSUB anyway as if they were legal, and only by chance did they match an isel pattern anyway.

The target can get the old behavior back by simply handling STRICT_FADD and STRICT_FSUB directly (presumably also via Custom operations similar to ones it uses for FADD/FSUB), so this is only a "regression" for the current fallback implementation.

Another thought...

Ok, I think we're pretty close to having unary FNeg optimizations in line with binary FNeg optimizations. Is anyone aware of any obvious passes that I've missed?

In D65226#1599743, @kpn wrote:

It looks like we're unrolling some vectors where before we weren't. That seems unfortunate. Is that the reason for the generated code quality regressions?

Makes sense to me. As I recall, the mutation was always intended as a temporary solution.

In D64412#1587016, @uweigand wrote:

This patch would add the additional option of also changing the relative order of the two strict_fmul operations.

I think I'll have to challenge you a little here. ;)

Bah, my last comment was flawed! I read the test cases incorrectly and missed the 'fpexcept.ignore' on some of them.

I'm reviewing the trap-safety issues now and have some open questions (language lawyers needed??). Let's say we have something like this:

Is there any chance I can get this patch into 9? What do I need to do to make that happen?

In D63782#1579595, @efriedma wrote:

How aggressive is LLVM's UB handling? Would it remove an entire block/function if UB is found in it?

If LLVM can prove a basic block unconditionally executes UB, it will be erased. But "unconditionally" is an important qualifier. For example, consider the following function: void f(void g()) { g(); *(int*)0 = 0; }. The call to g isn't erased because we can't prove g will return.

In D63782#1573780, @kpn wrote:

In D63782#1561585, @andrew.w.kaylor wrote:

I might be opening a can of worms here and I'm not a language expert, but it isn't clear to me from reading the C99 standard that defining fptosi/fptoui as returning poison values in the unrepresentable case allows correct implementation of the C standard. That is, it doesn't seem to me that the standard actually says this is undefined behavior. It just says the resulting value is unspecified, and the exception behavior is explicitly defined. On the other hand, C++ does say clearly that it is undefined behavior, right?

In D54649#1561237, @uweigand wrote:

Now that this has landed, are you planning to have another look at this?

This LGTM at a high level, but I don't fully understand the AVR concerns from D63294. Maybe @eli.friedman would be a better reviewer?

In D63782#1560136, @craig.topper wrote:

In D63782#1560123, @cameron.mcinally wrote:

So pragmatically, an invalid exception is an alarm that the code is off track. As long as the exception is handled appropriately (default or an alternative), the result of the invalid operation shouldn't matter. Whatever LLVM wants to do with the value gets no arguments from me, since we've already self-destructed (unless the program handles the exception gracefully, but that wouldn't require a defined result from the invalid operation anyway).

But the exception could be masked couldn't it?

In D63782#1559964, @efriedma wrote:

I think IEEE-754 does define this

Your citation doesn't actually specify what value is returned, only that an exception is raised.

In D63782#1559923, @efriedma wrote:

Unless I'm misunderstanding, we should be leaving the constrained converts alone until a hardware instruction is produced.

We have to define the semantics; I mean, I guess we could say "If the value cannot fit in the destination type, the result is computed in a target-specific way", but we'd have to state it explicitly. And it's sort of awkward.

In D63782#1558430, @efriedma wrote:

What happens if the input float is out of range? fptosi/fptoui instructions produce poison; not sure if you want that here.

In D63609#1556071, @spatel wrote:

Answering myself - doesn't look like anything has changed since:
D62444 / rL361788

Add two more tests (X86/propagate_ir_flags.ll).

LGTM, assuming the test writer was not intentionally playing games with the fabs(...) calls.

Ping*2.

Remove unintentional changes. Also fix up a poor naming choice.

Correct comment. Should be 'nnan', not 'nnan and nsz'.

In D63405#1546701, @spatel wrote:

But there's a separate question that is raised here: why is it legal to convert fneg to fsub -0.0? That loosens the IEEE requirement when dealing with a NAN. I'd think this should be legalized by converting to integer and flipping the sign bit (xor).
ping @cameron.mcinally