Page MenuHomePhabricator

[mlir][math] Expand coverage of atan2 expansion
ClosedPublic

Authored by jpienaar on Feb 3 2022, 9:21 PM.

Details

Summary

Reuse the higher precision F32 approximation for the F16 one (by expanding and
truncating).

Diff Detail

Event Timeline

jpienaar created this revision.Feb 3 2022, 9:21 PM
jpienaar requested review of this revision.Feb 3 2022, 9:22 PM

I think it's ok as a fallback for f16/f32, but maybe @mehdi_amini has a different opinion.

Mind changing the atan2 test to run on f16? It should handle all possible cases that way.

mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
424

Shouldn't this be > 32?

jpienaar updated this revision to Diff 406152.Feb 4 2022, 9:02 PM

Update test instance and use helper pattern

That's fine with me, I'd be wary that just using f16 blindly in the f32 approximation wouldn't be good enough in terms of precision and that a different expansion would be needed there. This seems safer and we can optimize this for f16 if needed in the future.

jpienaar updated this revision to Diff 406187.Feb 5 2022, 9:16 AM

Generalize helper a bit so that it could be reused for other fallback approximations.

mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
424

Indeed :) (I moved these into a helper pattern as that could be made into something a bit more reusable)

jpienaar edited the summary of this revision. (Show Details)Feb 8 2022, 6:40 AM
jpienaar added a reviewer: mehdi_amini.
ezhulenev accepted this revision.Feb 8 2022, 8:56 AM
This revision is now accepted and ready to land.Feb 8 2022, 8:56 AM
This revision was automatically updated to reflect the committed changes.