Skip to content

Emit tosa::erf during lowering gelu op #4151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vinitdeodhar
Copy link

@vinitdeodhar vinitdeodhar commented Apr 22, 2025

The PR adds support to use tosa::erf during lowering of aten.gelu op
Currently, aten.gelu uses its own custom implementation of erf which was designed before tosa support for tosa::erf : llvm/llvm-project@1fef1f9

aten.erf is already mapped to tosa::ErfOp https://github.com/vinitdeodhar/torch-mlir/blob/c785435a049a694a1814a7304ed0c34abe8b2580/lib/Conversion/TorchToTosa/TorchToTosa.cpp#L9176

The two implementations (Existing aten.gelu erf and tosa::erf) produce numerically different results.

tosa::erf is more precise approximation of the erf function. tosa::erf approximation is based on the following stackoverflow post:
https://stackoverflow.com/questions/35966695/vectorizable-implementation-of-complementary-error-function-erfcf
The stackoverflow post is in turn based on:
M. M. Shepherd and J. G. Laframboise, "Chebyshev Approximation of
(1+2x)exp(x^2)erfc x in 0 <= x < INF", Mathematics of Computation, Vol. 36,
No. 153, January 1981, pp. 249-253.
Maximum error: 2.65 ulps

Copy link
Member

@sahas3 sahas3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good cleanup to just use tosa.erf. Can you please add an e2e test to lock down the numeric that is failing with the current implementation but passes with the change?

@sahas3 sahas3 requested a review from sjarus April 22, 2025 18:05
Copy link
Collaborator

@sjarus sjarus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I echo @sahas3's request - LGTM but would like to see an e2e test for the fx_importer_tosa target.

@vinitdeodhar
Copy link
Author

I echo @sahas3's request - LGTM but would like to see an e2e test for the fx_importer_tosa target.

Thanks for the feedback. Added e2e test which fails without and pass with the cleanup changes

@vinitdeodhar
Copy link
Author

Since I dont have access, can someone help land this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants