[PATCH] D85504: [Reassociate] [PowerPC] stop common out mul factors if fma is preferred on target

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

I'm not sure why i'm added as a reviewer on this patch, i don't work on ppc, i never used it, and i never really worked on that pass.

In D85504#2202066 <https://reviews.llvm.org/D85504#2202066>, @shchenz wrote:

> In D85504#2202000 <https://reviews.llvm.org/D85504#2202000>, @lebedev.ri wrote:
>
>> (can you please maybe spellcheck both the patch description and the wording within the patch? it is really hard to read)
>
> Done
>
>> I'm not really sure this is the way forward.
>>
>> 1. This is a target-agnostic pass, it we add arbitrary restrictions it will likely miss some intended transforms
>
> Could you be more specific? I verified this change on PowerPC for some benchmarks, no degradation found.

Perhaps something like `a*b * c*d + e*f * c*d`, surely it should be `(a*b + e*f) * c * d`,
but won't this patch keep it as `a*b*c*d + e*f*c*d`?

>> 2. As usual, just preventing some transform is not the solution, because you can still have "bad" input that will still be lowered badly
>
> Agree. But if we know that one transformation has no benefit at all or it has a negative impact on some specific targets, should we stop the transformation on these targets? Reversing the transformations back in later pass will make compiler spend compiling time both on the harmful transformations and on the reversed transformations? also I am worried about the feasibility of reversing this "complex" transformation in later pass like DAGCombine...





================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:1500
     if (Ops.size() == 2 &&
         (match(TheOp, m_Neg(m_Value())) || match(TheOp, m_FNeg(m_Value()))))
       return Constant::getNullValue(X->getType());
----------------
So we also prevent the transform for integers now?
But there's no FMA for them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85504