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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1833
+  case Intrinsic::matrix_multiply: {
+    // Optimise negation in matrix multiplication.
+    // If we have a negated operand where it's size is larger than the second
----------------
optimise -> optimize (US spelling is commonly used in LLVM)


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1835
+    // If we have a negated operand where it's size is larger than the second
+    // operand or the result We can optimise the result by moving the negation
+    // operation to the smallest operand in the equation This covers two cases:
----------------
nit: `We -> we`, `optimise -> optimize` (US spelling is commonly used in LLVM)


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1837
+    // operation to the smallest operand in the equation This covers two cases:
+    // Case 1: the operand has the smalest element count i.e
+    // (-A) * B = A * (-B)
----------------
`smalest->smallest`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1846
+
+    VectorType *RetType = dyn_cast<VectorType>(II->getType());
+    Instruction *FNegOp;
----------------
no need to `dyn_cast` here, the result is guaranteed to be a vector type, `cast` can be used.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1850
+    unsigned SecondOperandArg;
+    bool MatchOp0 = match(Op0, m_FNeg(m_Value(X)));
+    bool MatchOp1 = match(Op1, m_FNeg(m_Value(X)));
----------------
no need to capture a value you are not using , `m_Value()` should just work

Also, you only use the variable `MatchOp0` in a single place, so it would be easier to read if you just use `if (match(..))`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1884
+      replaceInstUsesWith(*FNegOp, OpNotNeg);
+      // Insert after call instruction
+      Builder.SetInsertPoint(II->getNextNode());
----------------
nit: Period at end of sentence.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1886
+      Builder.SetInsertPoint(II->getNextNode());
+      Instruction *FNegInst = cast<Instruction>(Builder.CreateFNeg(II));
+      replaceInstUsesWith(*II, FNegInst);
----------------
There should be no need to cast to `Instruction` here?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1916
   }
+
   case Intrinsic::fma: {
----------------
please remove the stray line change.


================
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));
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133300