[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;
+    }
----------------
andreadb wrote:
> 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.
> 
Sorry, the first sentence is incorrect. It should have been "So, you are creating a copy of this".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72480