Related to https://reviews.llvm.org/D73556.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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: (Here and below) | |
7 | Is there a reason to have this name attribute? (Here and below) |
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.