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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1834
   }
   case Intrinsic::fmuladd: {
     // Canonicalize fast fmuladd to the separate fmul + fadd.
----------------
Add an FP transform near these other FP intrinsics.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3277
+                                       RetType->getElementCount())) {
+      replaceInstUsesWith(*FNegOp, OpNotNeg);
+      // Insert after call instruction
----------------
fhahn wrote:
> I think this is not correct, you are updating all uses of `FNegOp`, but we only have to update the use in the matrix multiply. Can you add a test case where the `FNeg` also has other users in some different instructions? They should remain unchanged.
> 
> Also, it would probably make sense to limit this to `fneg` instructions with a single use. If there are other uses outside the multiply, we still need to negate the input and we only add an extra `fneg`.
Also, we don't typically replace operands - just create a new call. That's what instcombine's worklist iteration is expecting.
Look at existing FP transforms above for examples (and where I think this transform should be placed too).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133300