[PATCH] D74703: Add test op for quantizer statistics.

[Stella Laurenzo via Phabricator via llvm-commits]

stellaraccident requested changes to this revision.
stellaraccident added a comment.
This revision now requires changes to proceed.

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



================
Comment at: mlir/include/mlir/Dialect/QuantOps/QuantOps.td:241
+  let summary =
+      "Extracts per-layer or per-axis tensor statistics by quantizer.";
+
----------------
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.


================
Comment at: mlir/lib/Dialect/QuantOps/IR/QuantOps.cpp:18
 #include "mlir/IR/StandardTypes.h"
+#include "mlir/Quantizer/Support/Statistics.h"
 #include "llvm/ADT/StringRef.h"
----------------
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.


================
Comment at: mlir/test/Quantizer/stats.mlir:5
+// CHECK-LABEL: @per_layer
+// CHECK: %cst = constant dense<[1.000000e+00, 1.200000e+01, 6.500000e+00, 0.000000e+00]> : tensor<4xf64>
+func @per_layer() -> tensor<4xf64> {
----------------
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)


================
Comment at: mlir/test/Quantizer/stats.mlir:7
+func @per_layer() -> tensor<4xf64> {
+  %cst = constant {name = "layer"} dense<[[[1.0, 2.0], [3.0, 4.0], [5.0, 6.0]], [[7.0, 8.0], [9.0, 10.0], [11.0, 12.0]]]> : tensor<2x3x2xf32>
+  %result = "quant.quantizer_stats"(%cst) : (tensor<2x3x2xf32>) -> tensor<4xf64>
----------------
Is there a reason to have this name attribute? (Here and below)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74703