[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:1834
+    // Optimize negation in matrix multiplication.
+    // If We have a negated operand where it's size is larger than the second
+    // operand or the result We can optimize the result by moving the negation
----------------
nit: `We -> we`, `it's -> its`.

On second thought, I think it would be better to just move the `Case 1,2,3` comments to the code that actually deals with those cases and remove the sentence here.


================
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 optimize the result by moving the negation
+    // operation to the smallest operand in the equation:
----------------
`We -> we`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1872
+    VectorType *SecondOperandType = cast<VectorType>(SecondOperand->getType());
+    if (ElementCount::isKnownGT(FNegType->getElementCount(),
+                                SecondOperandType->getElementCount()) &&
----------------
IIUC this is the case where we move the negation from one to the other operand. Could you move the comment for `Case 2` above here?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1877
+
+      Value *InverseSecondOp = Builder.CreateFNeg(SecondOperand);
+      replaceOperand(*II, NegatedOperandArg, OpNotNeg);
----------------
If I read the code correctly, this may not be the second operand but could also the first one if the second one is negated?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1878
+      Value *InverseSecondOp = Builder.CreateFNeg(SecondOperand);
+      replaceOperand(*II, NegatedOperandArg, OpNotNeg);
+      replaceOperand(*II, SecondOperandArg, InverseSecondOp);
----------------
I thought an earlier version created a new call here, rather than updating the exist one. Did we agree that `replaceOperand` here is the right thing to do?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1882
+    }
+    if (ElementCount::isKnownGT(FNegType->getElementCount(),
+                                RetType->getElementCount())) {
----------------
IIUC this is the case where we move the negation from an argument to the result of the multiply. Could you move the comment from `Case 3` here?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1887
+      Builder.SetInsertPoint(II->getNextNode());
+      Instruction *FNegInst = cast<Instruction>(Builder.CreateFNeg(II));
+      replaceInstUsesWith(*II, FNegInst);
----------------
there should be no need to cast to `Instruction` here, you should be able to just use `Value`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133300