Page MenuHomePhabricator

[Matrix] Add info about number of operations to remarks.
ClosedPublic

Authored by fhahn on Jan 9 2020, 2:03 PM.

Details

Summary

This patch updates the remark to also include a summary of the number of
vector operations generated for each matrix expression.

Diff Detail

Event Timeline

fhahn created this revision.Jan 9 2020, 2:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2020, 2:03 PM

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

fhahn updated this revision to Diff 237388.Jan 10 2020, 10:32 AM

Handle re-used expressions.

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

andreadb added inline comments.
llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
152–158

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.

andreadb added inline comments.Jan 16 2020, 6:35 AM
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".

fhahn updated this revision to Diff 238566.Jan 16 2020, 12:02 PM

Replace OpInfoTy::add with a += operator implementation.

fhahn marked an inline comment as done.Jan 16 2020, 12:03 PM
fhahn added inline comments.
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

anemet accepted this revision.Jan 23 2020, 11:50 AM

LGTM!

llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
144

Add a comment please.

This revision is now accepted and ready to land.Jan 23 2020, 11:50 AM
This revision was automatically updated to reflect the committed changes.