Both the ASan build:

Fix incoming. Sorry for not running the ASan tests...

Apologies, @pcc. Looking now. Will revert if it's not obvious...

Recommitted this patch with Ocaml test fix. I was not able to find quick-start documentation for the Ocaml bindings, so the Ocaml failure has not been tested. That said, the failure mode seems extremely low risk. Will monitor the buildbots for problems...

@gribozavr I see that you also reverted @RKSimon's commit for the OCaml/core.ml failure:

In D61675#1703749, @cameron.mcinally wrote:

I reverted it in r374354. Please test before re-landing.

Hmm, how do I run those tests? I did not see that failure with check-all.

That's a pretty straightforward failure. It's just a one-line IR change:

fsub float {{.*}}0{{.*}}, %F1 -> fneg float %F1

I reverted it in r374354. Please test before re-landing.

This looks good to me, but other users should review it as well...

Sorry for the slow response. I've been busy with other projects.

Looks pretty good. This could probably use some tests in TypeAndConstantValueParsing of llvm/unittests/AsmParser/AsmParserTest.cpp. In particular, an isa<VScale>(...) test would be good. And maybe a negative test to make sure something like <undef x 2 x f64> can't be parsed.

What happens if we see IR like:

LGTM with a moderate confidence level. Maybe give other reviewers a day or so to comment before committing.

LGTM

Would it be better to do an attributes #0 = { ...} style change? E.g.:

For posterity's sake, @andrew.w.kaylor just suggested adding a nftz fast math flag:

Some data points...

Address review by @lenary.

In D61675#1671688, @cameron.mcinally wrote:

Someone could argue, though, to just use a constrained strict FSub if you care about DAZ/FTZ. That seems like a valid solution. But, that is really treating DAZ/FTZ like a rounding-mode, not underflow. It would be a heavy hammer to enforce all the side-effect concerns when the user really only cares about DAZ/FTZ. In other words, we'll be losing significant performance in an attempt to gain significant performance.

In D61675#1671602, @spatel wrote:

And I think the case where a user changes MXCSR bits is UB for C/C++ ( https://bugs.llvm.org/show_bug.cgi?id=8100#c15 ). So x86 never has a problem in theory, but it might in practice because users believe that twiddling MXCSR bits is allowed?

My confidence is somewhat low though...

In D61675#1670603, @spatel wrote:

On 2nd thought, that doesn't really make sense for CSE. The real question is whether we should canonicalize the binary fneg to unary fneg in instcombine. And should that happen before/after/concurrent with this change to clang?

Maybe a dumb question, but what happens if two incoming PHI values have different FMFs set? Does the PHI's FMF override those? If so, is that safe?

In D67360#1667091, @uweigand wrote:

My understanding was that this is a temporary restriction; in the end, the goal is to allow mixing constrained an non-constrained operations, once we have the necessary barriers. Did I misunderstand this?

I've been digging through this pass and it seems to be ok AFAICT. OptimizeInst(...) canonicalizes both unary and binary FNegs to -1.0*X, if they are fast and part of a special multiply tree. Other FNegs end up as leaf nodes, so no problem there.

LGTM

LGTM.

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?