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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn 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:
> 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".
Thanks for taking a look! You are right, having a += operator would make more sense. I've updated the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72480