[PATCH] D80801: [DAGCombiner] allow more folding of fadd + fmul into fma

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D80801#2063810 <https://reviews.llvm.org/D80801#2063810>, @rscottmanley wrote:

> > Not sure if I'm understanding the question. Is there a target or a code pattern with a known disadvantage for the 2 fma variant?
>
> I mostly agree that what you're proposing is an improvement over what's currently there, but my question is that if you're gonna change it, is it really better to do what you're suggesting compared to creating instructions that can be executed in parallel? There are certainly performance differences between fma(a,b,fma(c,d,n)) and a*b + fma(c,d,n) depending on the target.


Ah - I missed that you are rearranging the operands to make the fmul independent from the fma. I agree that could be better depending on the target and surrounding code.

But we need some detailed target model info to make that transform *over* this one. We have that info in MachineCombiner, and I think the scenario you are suggesting is at least partly why the TLI hook for that already exists. It's also possible to expand this back out to separate fmul/fadd in that pass.

Given the constraints in SDAG, we should choose the (fma(fma)) variant by default (assuming as we do here that the target has fma instructions). For example on x86, our best perf heuristic at this stage of compilation on any recent Intel or AMD core is number of uops. The option with separate fmul and fadd always has more uops, so it would be backwards to choose that sequence here and then try to undo that later.

If this is the wrong choice for some other target, they can can still opt out, so I think this is a safe option. Alternatively, we could make the enableAggressiveFMAFusion() hook more nuanced by changing the bool return to a enum'd level of aggression, and then deciding which of the current transforms under here require a higher level of fma perf.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80801/new/

https://reviews.llvm.org/D80801