This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add path for math.round to spirv for OCL and GLSL
ClosedPublic

Authored by rsuderman on Jul 6 2022, 3:51 PM.

Details

Summary

OpenCL's round function matches math.round so we can directly lower to
the op, this includes adding the op definition to the SPIRV OCL ops.
GLSL does not guarantee rounding direction so we include custom rounding
code to guarantee correct rounding direction.

Diff Detail

Event Timeline

rsuderman created this revision.Jul 6 2022, 3:51 PM
rsuderman requested review of this revision.Jul 6 2022, 3:51 PM
rsuderman updated this revision to Diff 442707.Jul 6 2022, 3:52 PM

Removed unintentional inclusion.

antiagainst requested changes to this revision.Jul 6 2022, 5:05 PM

Nice, thanks for supporting this, @rsuderman!

mlir/lib/Conversion/MathToSPIRV/MathToSPIRV.cpp
237

Nit: ... to GLSL SPIR-V extended ops.

260

Hmm, this is using expensive FMul and FDiv ops. What about using FAdd like the expansion pattern in the below?

mlir/lib/Dialect/Math/Transforms/ExpandPatterns.cpp
57

Missing a test for this?

68

This should be math::FloorOp?

133

This should be exposed in the .h file?

This revision now requires changes to proceed.Jul 6 2022, 5:05 PM
rsuderman updated this revision to Diff 442730.Jul 6 2022, 5:15 PM

Moving extraneous code.

rsuderman marked 3 inline comments as done.Jul 6 2022, 5:15 PM
rsuderman added inline comments.
mlir/lib/Dialect/Math/Transforms/ExpandPatterns.cpp
57

This was some extraneous changes that leaked in. Just removed it entirely.

68

Ditto on removed entirely.

133

Ditto on the removal.

rsuderman updated this revision to Diff 442731.Jul 6 2022, 5:17 PM
rsuderman marked 3 inline comments as done.

Updated glsl comment

rsuderman marked 2 inline comments as done.Jul 6 2022, 5:19 PM
rsuderman added inline comments.
mlir/lib/Conversion/MathToSPIRV/MathToSPIRV.cpp
237

Fixed.

260

I discovered an error with the add that causes unintentional double rounding. Specifically the case of 0.49999997f + 0,5f -> 1.0.

I can see if there is an alternative solution? One option is to do more bit magic instead but my f32 mantissa manipulations are not fantastic.

antiagainst accepted this revision.Jul 7 2022, 11:07 AM
antiagainst added inline comments.
mlir/lib/Conversion/MathToSPIRV/MathToSPIRV.cpp
260

Ha, interesting. Okay that makes sense then. Could you add comments in the code explaining this so later when we come back to this we can still understand why doing mul & div?

This revision is now accepted and ready to land.Jul 7 2022, 11:07 AM
rsuderman updated this revision to Diff 443026.Jul 7 2022, 12:12 PM
rsuderman marked 2 inline comments as done.

Updated to use select op instead

rsuderman marked an inline comment as done.Jul 7 2022, 12:18 PM
rsuderman added inline comments.
mlir/lib/Conversion/MathToSPIRV/MathToSPIRV.cpp
260

Changed to use a select operation. It is slightly more ops but should be computationally faster.

This revision was landed with ongoing or failed builds.Jul 7 2022, 12:27 PM
This revision was automatically updated to reflect the committed changes.