This patch updates the remark to also include a summary of the number of
vector operations generated for each matrix expression.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Unit tests: fail. 61731 tests passed, 1 failed and 779 were skipped.
failed: LLVM.Transforms/LowerMatrixIntrinsics/remarks.ll
clang-tidy: fail. Please fix clang-tidy findings.
clang-format: pass.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Unit tests: pass. 61764 tests passed, 0 failed and 780 were skipped.
clang-tidy: fail. Please fix clang-tidy findings.
clang-format: pass.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp | ||
---|---|---|
152–158 | So, you are creating a copy of Other. 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". |
llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp | ||
---|---|---|
152–158 | Sorry, the first sentence is incorrect. It should have been "So, you are creating a copy of this". |
llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp | ||
---|---|---|
152–158 | Thanks for taking a look! You are right, having a += operator would make more sense. I've updated the patch. |
Unit tests: pass. 61931 tests passed, 0 failed and 783 were skipped.
clang-tidy: unknown.
clang-format: pass.
Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Add a comment please.