[PATCH] D158079: [InstCombine] Contracting x^2 + 2*x*y + y^2 to (x + y)^2 (float)

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1059
+                                       m_FMul(m_Deferred(B), m_Deferred(B))))));
+  }
+
----------------
rainerzufalldererste wrote:
> rainerzufalldererste wrote:
> > goldstein.w.n wrote:
> > > This match code is basically identical to `foldSquareSumInts`. The only difference other than `FMul` vs `Mul` is you do match `FMul(A, 2)` for floats and `m_Shl(A, 1)` for ints.
> > > Can you make the match code a helper that takes either fmul/2x matcher (or just lambda wrapping) so it can be used for SumFloat / SumInt?
> > Does that imply that `m_c_FAdd` can simply be replaced with `m_c_Add` and will continue to match properly for floating point values as well?
> > I presume that would entail partially matching another pattern and then deferring the actual check for the mul2 match, as `BinaryOp_match<RHS, LHS, OpCode>` would have different `OpCode`s for `FMul` and `Shl`, which sounds like a huge mess to me; or is there a cleaner way to do that?
> > 
> > Something like this sadly doesn't compile (as the lambda return type is ambiguous):
> > ```
> >   const auto FpMul2Matcher = [](auto &value) {
> >     return m_FMul(value, m_SpecificFP(2.0));
> >   };
> >   const auto IntMul2Matcher = [](auto &value) {
> >     return m_Shl(value, m_SpecificInt(1));
> >   };
> >   const auto Mul2Matcher = FP ? FpMul2Matcher : IntMul2Matcher;
> > ```
> Even something like this shouldn't work.
> 
> ```
> template <typename TMul2, typename TCAdd, typename TMul>
> static bool MatchesSquareSum(BinaryOperator &I, Value *&A, Value *&B,
>                              const TMul2 &Mul2, const TCAdd &CAdd,
>                              const TMul &Mul) {
> 
>   // (a * a) + (((a * 2) + b) * b)
>   bool Matches =
>       match(&I, CAdd(m_OneUse(Mul(m_Value(A), m_Deferred(A))),
>                      m_OneUse(Mul(CAdd(Mul2(m_Deferred(A)), m_Value(B)),
>                                   m_Deferred(B)))));
> 
>   // ((a * b) * 2)  or ((a * 2) * b)
>   // +
>   // (a * a + b * b) or (b * b + a * a)
>   if (!Matches) {
>     Matches =
>         match(&I, CAdd(m_CombineOr(m_OneUse(Mul2(Mul(m_Value(A), m_Value(B)))),
>                                    m_OneUse(Mul(Mul2(m_Value(A)), m_Value(B)))),
>                        m_OneUse(CAdd(Mul(m_Deferred(A), m_Deferred(A)),
>                                      Mul(m_Deferred(B), m_Deferred(B))))));
>   }
> 
>   return Matches;
> }
> ```
> 
> I agree that it's messy to have duplicate code, but with the way op-codes are used as template parameters I don't see a way without template specialization to do this nicely; and with template specialization it's even more of a beast.
> Am I missing some obvious way built into llvm/InstCombine to do this nicely?
Why doesn't that code work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158079