This is an archive of the discontinued LLVM Phabricator instance.

Add test op for quantizer statistics.
Needs RevisionPublic

Authored by Vooblin on Feb 17 2020, 4:05 AM.

Details

Diff Detail

Event Timeline

Vooblin created this revision.Feb 17 2020, 4:05 AM
Vooblin edited the summary of this revision. (Show Details)Feb 17 2020, 4:07 AM
Vooblin added a reviewer: stellaraccident.
Vooblin added a subscriber: denis13.
stellaraccident requested changes to this revision.Mar 10 2020, 6:17 AM

Nice, thanks. Some small changes requested. Also, I would have expected cmake updates since you added a dep.

mlir/include/mlir/Dialect/QuantOps/QuantOps.td
241

Imo, fine to leave it the "by quantizer" suffix.

Since I don't think this op will be used as-is outside of tests, lets note it as such in the description. Also note what code paths this is exercising.

I think that renaming it "TestComputeStats" is more descriptive (includes "test" and outside parties don't necessarily care that this comes from the quantizer support library.

mlir/lib/Dialect/QuantOps/IR/QuantOps.cpp
18

Note for future refactoring: we currently have some support code in the QuantOps dialect and some in the quantizer. We should pick one and move the others.

mlir/test/Quantizer/stats.mlir
5

We don't usually hardcore individual ssa names in tests (there may still be old ones that do). Prefer something like:
%[[CST:.+]] =

(Here and below)

7

Is there a reason to have this name attribute? (Here and below)

This revision now requires changes to proceed.Mar 10 2020, 6:17 AM