[PATCH] D80175: [PowerPC][MachineCombiner] reassociate fma to expose more ILP

Thu May 28 05:57:28 PDT 2020

spatel added a comment.

In D80175#2058915 <https://reviews.llvm.org/D80175#2058915>, @shchenz wrote:

> In D80175#2058335 <https://reviews.llvm.org/D80175#2058335>, @spatel wrote:
>
> > I did not look at the patch itself except to notice that it is a lot of code...so I have to ask - did you look at implementing at least the 1st pattern in DAGCombiner? That seems like a general improvement for any superscalar micro-arch with no register pressure disadvantage.
>
>
> Thanks for looking into this @spatel
>
> Yes, I tried to implement pattern 1 in DAGCombiner, but I got some LIT failures related to register allocation on platform AArch64 and Thumb2. And this kind of opt will increase register pressure. I think it is better not to add it in DAGCombiner.
>  Reason I add these two patterns in MachineCombiner is:
>  1: This pass is targeted for ILP related optimization
>  2: Adding register pressure estimation model here should be easy than in DAGCombiner. We can do similar estimation like we did in MachineLICM if we want to model it in future?
>  3: These two patterns have to be put together. After breaking pattern 2: (fma+fma+fma) to (fmul+fma+fma+fadd), the last `fadd` can be combined with following two fmas as pattern 1, and we can get more paralleled fmas.

Thank you for the explanation. I agree that this is a better place to try the transform. DAGCombiner has no real register pressure analysis. One thing to be aware of: the compile-time cost of MachineCombiner was potentially high when analyzing large blocks. My experience with this pass is a few years old, so this might have changed, but there may be some bugzilla reports on that.

If you have a version of the transform as a DAGCombiner patch and can post it somewhere, I would still be interested in trying it out locally.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80175