- User Since
- Jul 18 2017, 1:54 PM (130 w, 3 d)
Thu, Jan 16
I should do the other DAG combiner fma changes after this is wrapped up.
Wed, Jan 15
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.
Tue, Jan 14
Looked at this internally for us and looks ok. LGTM.
Thu, Dec 19
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.
Nov 20 2019
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.
Nov 7 2019
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...
Oct 14 2019
Oct 11 2019
Oct 9 2019
Sep 13 2019
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.
Aug 22 2019
Follow on note from D66612, I am testing the rest of the Reassociate.cpp changes that were talked about there(+some tests as well). Cameron, do you want to incorporate those changes here?
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.
Aug 20 2019
Cameron here is an example with the issue:
Aug 19 2019
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.
Aug 9 2019
Aug 8 2019
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.
Jul 31 2019
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.
Jul 30 2019
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.
Jul 29 2019
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 wrt? 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,
Jul 24 2019
updated with one case...
Jul 23 2019
Jul 15 2019
Looks ok to me, but I would like some of the others to chime in first.
Jul 10 2019
Jul 9 2019
Jun 17 2019
With cloned tests for FMF.
There was an existing test, here it is with the modification for FMF
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 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.
Jun 14 2019
I was just about to change the test to assembler check test, not using the assert path...
Jun 3 2019
May 28 2019
May 1 2019
LGTM, thanks Sanjay
Apr 30 2019
Apr 18 2019
Feb 6 2019
Feb 5 2019
Migrated flag copy utility function to MachineInstr as a static function.
Feb 4 2019
Feb 1 2019
Jan 28 2019
Dec 18 2018
Dec 17 2018
Updated for posterity with the post review changes...
Dec 16 2018
As requested, add one more component for copy ir flags in to this review
Dec 13 2018
Oct 30 2018
Sep 19 2018
The fixed test for WebAssembly is in rL342598.
Ok, looking into it now.
Sep 13 2018
Changed copyIRFlags as suggested...