Let some of the others also comment, but on the surface it's what I would expect.

LGTM. My read on this change is that you have set it up to never make more code than we have, and transformation is safe.

From discussion, it seems we are fine with fmul and constant folding the way it is versed in the code today (unrevised).

Ping, 2nd time...

Ping.

All in all LGTM, will check back in bit to see what others have to say.

I should do the other DAG combiner fma changes after this is wrapped up.

We crossed that bridge internally at Apple a while ago, meaning I have some code debt for cleaning up open source for fma formation that uses contract and reassoc differently than we do today, both together and separately, case by case.

Looked at this internally for us and looks ok. LGTM.

I like the notion of avoiding/reducing intersect context hits by placing it in the vector reduction scope (line 3128).

When I looked at this recently as well, it looked like intersect was were we lost our flags.

For us this would be an impediment as we have math models that want ieee behavior while relaxing precision. Adding nnan or ninf would obstruct those choices.

It is a step in the right direction. I tried it out internally and its fine for us. We too have work to do wrt phi mapping in GiSel wrt flags. So initially, LGTM. You might want to see what others say and so leave it for bit...

WRT divergent phi FMF, if you wanted to be pessimistic you could do an intersection(and), doing an inclusion(or) might include too much. Either way behavior would change. The intersection already has other prior context in FMF.

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

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.

LGTM, but I will give Sanjay some time before moving forward.

Cameron here is an example with the issue:

Just putting a place holder for NegateValue in Reassociate.cpp to be updated for both binary and unary context when examining the uses of the Value V. In fact the full file should be looked over for other cases as well. I tried out the current patch and we hit the first case pretty quickly internally.

Just to close the loop, the issues I saw are internal manifested only, they do not port to other targets.

Tell you what, I don't want to hold up this patch. I will see if I can abstract a test example into common form for a public target with an example or two and send it to you offline.

We have some stir(some regressions - a few instructions here and there), however the change is overall beneficial for us.

I worked through some of the tests, but would like another set of eyes to verify. Doing a quick internal validation with this for regressions.

added a TODO comment for machine combines in test/CodeGen/AArch64/fadd-combines.ll,
test/CodeGen/PowerPC/fma-mutate.ll, test/CodeGen/PowerPC/qpx-recipest.ll, test/CodeGen/PowerPC/recipest.ll, test/CodeGen/X86/fp-fold.ll now have just standard CHECK lines. The fmf contract flag was
removed from test/CodeGen/X86/dagcombine-unsafe-math.ll and test/CodeGen/X86/fp-fast.ll.

Note: test/CodeGen/AMDGPU/fneg-combines.ll needs rearchitecting, so i left it in options flag form, test/CodeGen/PowerPC/fmf-propagation.ll has portions that can be removed once we stop using the options flags and so i am leaving it in its current form with mods until that happens. test/CodeGen/X86/fp-fast.ll uses a subset of fmf that equate to the options flag that were used (let me know if you just want to generalize to fast or smaller subset), all the others use either fast or context relevant fmf and have been converted to not use options flags. Have a look and see what needs editing, currently this all passes testing.

The new code is actually better by 1 instruction, we just never completed the full match on the test. In the old path we had -enable-no-signed-zeros-fp-math on but no way to reach it for the zero fold of the fadd via llc as the flags are all user controlled. This should not be a regression.

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

Turning off inst combining fneg opts does not make things better for us however. It seems to consistently cause additional regressions. We can live with the new change then.

General comment: For us internally at Apple this does cause regressions, its small but there. Mostly because fneg becomes a side effect for fp arithmetic ops for us. I would request a target info guard which we can turn off to avoid the drift. If not, this is small enough to live with.

updated with one case...

Looks ok to me, but I would like some of the others to chime in first.

With cloned tests for FMF.

There was an existing test, here it is with the modification for FMF

Ok.

So I agree, but I think there should be a target specific combine with guards for Unsafe and nsz and this legalization case (for those who do not have it).

For your target. But as a catch all for legalization we should do this.

We do this as a combine earlier in target specific code under this constraint. Matt, perhaps you just missing that combine in GlobalIsel?

I am saying you have it right Matt for using MI flags, but we should guard folding based on Unsafe or nsz in the Flags.

SDAG fails isNegatibleForFree if that constraint is not met and we do not translate.

Should we not also guard GlobslIsel translation in this case and avoid if not met?

Ok, with the constraint like in the DAG:

Seems reasonable to me.

I was just about to change the test to assembler check test, not using the assert path...

LGTM, thanks Sanjay

Migrated flag copy utility function to MachineInstr as a static function.