Page MenuHomePhabricator

[mlir] Add polynomial approximation for math::Log2
ClosedPublic

Authored by cota on Mar 9 2021, 12:18 PM.

Details

Summary
name                     old cpu/op  new cpu/op  delta
BM_mlir_Log2_f32/10       134ns ±15%    45ns ± 4%  -66.39%  (p=0.000 n=20+17)
BM_mlir_Log2_f32/100     1.03µs ±16%  0.12µs ±10%  -88.78%  (p=0.000 n=20+18)
BM_mlir_Log2_f32/1k      10.3µs ±16%   0.7µs ± 5%  -93.24%  (p=0.000 n=20+17)
BM_mlir_Log2_f32/10k      104µs ±15%     7µs ±14%  -93.25%  (p=0.000 n=20+20)
BM_eigen_s_Log2_f32/10   95.3ns ±17%  90.9ns ± 6%     ~     (p=0.228 n=20+18)
BM_eigen_s_Log2_f32/100   907ns ± 3%   911ns ± 6%     ~     (p=0.539 n=16+20)
BM_eigen_s_Log2_f32/1k   9.88µs ± 4%  9.85µs ± 3%     ~     (p=0.790 n=16+17)
BM_eigen_s_Log2_f32/10k   105µs ±10%   110µs ±16%     ~     (p=0.459 n=16+20)
BM_eigen_v_Log2_f32/10   32.5ns ±31%  33.9ns ±14%   +4.31%  (p=0.028 n=17+20)
BM_eigen_v_Log2_f32/100   176ns ± 8%   180ns ± 7%   +2.19%  (p=0.045 n=16+17)
BM_eigen_v_Log2_f32/1k   1.44µs ± 4%  1.50µs ± 9%   +3.91%  (p=0.001 n=16+17)
BM_eigen_v_Log2_f32/10k  14.5µs ±10%  15.0µs ± 8%   +3.92%  (p=0.002 n=16+19)

Diff Detail

Event Timeline

cota created this revision.Mar 9 2021, 12:18 PM
cota requested review of this revision.Mar 9 2021, 12:18 PM
cota updated this revision to Diff 329436.Mar 9 2021, 12:24 PM

Update D98282: [mlir] Add polynomial approximation for math::Log2

Wrap the benchmark results in `quotes```.

cota edited the summary of this revision. (Show Details)Mar 9 2021, 12:25 PM
ezhulenev added inline comments.Mar 9 2021, 12:35 PM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
283–284

Template names should be CamelCase just like any other type name.

284

Maybe just inherit template from OpRewritePattern and instantiate log/log2 rewrites from it?

cota updated this revision to Diff 329462.Mar 9 2021, 2:06 PM

s/LOG_OP/Op/

cota marked an inline comment as done.Mar 9 2021, 2:08 PM
cota added inline comments.
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
284

I am having trouble understanding what you are suggesting. Can you please elaborate?

mehdi_amini added inline comments.Mar 9 2021, 3:27 PM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
284

Can you document the API? (in particular the base2 parameter).

Also the convention in MLIR is to not put free functions in anonymous namespaces but have them static outside.

mlir/test/Dialect/Math/polynomial-approximation.mlir
12–13

Nit: this check is not useful considering the one above.

13

What is different here compared to the math.log two lines above? Did you mean math.log2 here?

22–23

Similarly, you can omit this check, it is redundant with above.

ezhulenev added inline comments.Mar 9 2021, 4:26 PM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
284
template<typename Op>
class LogApproximationBase : public OpRewritePattern<Op>

class Log2Approximation : public LogApproximationBase<math::Log2Op>
cota updated this revision to Diff 329702.Mar 10 2021, 10:09 AM
cota marked 6 inline comments as done.
  • Test fixes as suggested by mehdi_amini.
  • Log base class as suggested by ezhulenev.
mlir/test/Dialect/Math/polynomial-approximation.mlir
13

Yes, I meant log2. Fixed, thanks!

cota updated this revision to Diff 329703.Mar 10 2021, 10:11 AM
  • Fix typo in comment.
ezhulenev accepted this revision.Mar 10 2021, 11:14 AM
This revision is now accepted and ready to land.Mar 10 2021, 11:14 AM
This revision was landed with ongoing or failed builds.Mar 10 2021, 2:49 PM
This revision was automatically updated to reflect the committed changes.