[PATCH] D133300: [InstCombine] Matrix multiplication negation optimisation

[Zain Jaffal via Phabricator via llvm-commits]

zjaffal added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1850-1858
+    bool MatchOp0 = match(Op0, m_FNeg(m_Value(X)));
+    bool MatchOp1 = match(Op1, m_FNeg(m_Value(X)));
+    if (MatchOp0 && MatchOp1) {
+      Instruction *FNegOp0 = cast<Instruction>(Op0);
+      Instruction *FNegOp1 = cast<Instruction>(Op1);
+      replaceInstUsesWith(*FNegOp0, FNegOp0->getOperand(0));
+      replaceInstUsesWith(*FNegOp1, FNegOp1->getOperand(0));
----------------
spatel wrote:
> fhahn wrote:
> > zjaffal wrote:
> > > spatel wrote:
> > > > I don't think this is correct if an fneg has multiple uses (similar to the bug noted earlier, and I repeat my suggestion to create new instructions rather than modifying existing ones).
> > > > 
> > > > Please split this change and tests to its own review ahead of the original transforms in this patch. 
> > > We don't need this check anyways. 
> > > if both operands are negative then there is two cases:
> > > 1. Both of the negations will be moved to the result and then another pass will remove the negations
> > > 2. negation gets moved to an operand and then we have two negations on one operand which will be optimised as well.
> > > 
> > @zjaffal do we have a test case where both operands are negated and at least one of the fnegs has multiple uses?
> Correct me if I'm wrong:
> 1. When both ops are negated, we don't care what the relative sizes of the values are, we always want to use the non-negated source ops.
> 2. When both ops are negated, we don't care if those ops have other uses, we always want to use the non-negated source ops.
> 
> Either way, we need tests to exercise those patterns.
> 
> ```
> 
> define <4 x double> @matrix_multiply_v2f64_v2f64(<2 x double> %a, <2 x double> %b) {
>   %a.neg = fneg <2 x double> %a
>   %b.neg = fneg <2 x double> %b
>   %res = call <4 x double> @llvm.matrix.multiply.v4f64.v2f64.v2f64(<2 x double> %a.neg, <2 x double> %b.neg, i32 2, i32 1, i32 2)
>   ret <4 x double> %res
> }
> 
> ```
> Correct me if I'm wrong:
> 1. When both ops are negated, we don't care what the relative sizes of the values are, we always want to use the non-negated source ops.
I am not sure I understand the question fully but in the case of the two operands the first operand gets handled first and we don't check if it is the larger of both
> 2. When both ops are negated, we don't care if those ops have other uses, we always want to use the non-negated source ops.
Currently if fmul has other uses we will still use the negated ops in the multiplication
> 
> Either way, we need tests to exercise those patterns.
> 
> ```
> 
> define <4 x double> @matrix_multiply_v2f64_v2f64(<2 x double> %a, <2 x double> %b) {
>   %a.neg = fneg <2 x double> %a
>   %b.neg = fneg <2 x double> %b
>   %res = call <4 x double> @llvm.matrix.multiply.v4f64.v2f64.v2f64(<2 x double> %a.neg, <2 x double> %b.neg, i32 2, i32 1, i32 2)
>   ret <4 x double> %res
> }
> 
> ```
Yes I think I need to expand on the test cases where we have two operands.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133300