[PATCH] D72480: [Matrix] Add info about number of operations to remarks.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:150-156
+    OpInfoTy add(const OpInfoTy &Other) {
+      auto Result = *this;
+      Result.NumStores += Other.NumStores;
+      Result.NumLoads += Other.NumLoads;
+      Result.NumComputeOps += Other.NumComputeOps;
+      return Result;
+    }
----------------
So, you are creating a copy of `Other`.
Then modifying that copy.
Then returning it by copy. So, there are two copies involved in this computation.

Did you mean this to be a non mutating function? If so, then it should be marked as const. Otherwise, if the intention was to mutate the `this` fields then you don't need those copies.

I am writing this because - at least for what I can see - the only use of add() is at line 1208

```
      OpInfoTy Count = CM->second.getOpInfo();
      for (Value *Op : cast<Instruction>(Root)->operand_values())
        Count = Count.add(sumOpInfos(Op, ReusedExprs));
      return Count;
```

Since Count is assigned to by each iteration, you end up with yet another "copy".
But more importantly, maybe the intention was to make `add()` a mutating function.
In which case you could use a += operator instead.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72480