Skip to content

Conversation

@KJlaccHoeUM9l
Copy link
Contributor

ONNX Runtime supports FastGelu in its contrib opset (documentation and code). Implementation of this operator refers to an approximation for original GELU (article).
Previously, converter for Gelu was added to TVM ONNX frontend, however, there is no such converter for FastGelu.
This PR adds this converter to ONNX frontend.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Oct 18, 2022

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

  • No users to tag found in teams: onnx See #10317 for details
  • Built docs for commit 8f9ba0b can be found here.

Generated by tvm-bot

@KJlaccHoeUM9l
Copy link
Contributor Author

Hello @jwfromm!
Could you review this PR?

@areusch areusch added needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it and removed needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it labels Oct 19, 2022
@pytest.mark.parametrize("op_name", ["Gelu", "FastGelu"], scope="session")
@tvm.testing.parametrize_targets
def test_gelu(target, dev):
def test_gelu(target, dev, op_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also extend the tests to check accuracy for fp16 datatype.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines +947 to +948
const1 = _expr.const(math.sqrt(2 / math.pi), dtype=const_dtype)
const2 = _expr.const(0.044715 * math.sqrt(2 / math.pi), dtype=const_dtype)
Copy link
Contributor

Choose a reason for hiding this comment

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

I took a look at the ONNX documentation. And they use hard-coded constants in their implementation. Let's check that for fp16 we will have the same accuracy with the current implementation as ONNX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 953 to 954
const1 = _expr.const(0.7978845608028654, dtype=const_dtype) # sqrt(2.0 / PI)
const2 = _expr.const(0.0356774081363001, dtype=const_dtype) # 0.044715 * sqrt(2.0 / PI)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's return explicit calculation of these values if it is not affect on the accuracy

Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@KJlaccHoeUM9l
Copy link
Contributor Author

Hello @AndrewZhaoLuo!
Could you take a look at this PR?
Do you have any additional comments/suggestions?

@KJlaccHoeUM9l
Copy link
Contributor Author

@tvm-bot rerun

@AndrewZhaoLuo AndrewZhaoLuo merged commit 5400b94 into apache:main Oct 24, 2022
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 10, 2022
…opset (apache#13119)

* add converter for FastGelu from Microsoft onnxruntime contrib opset

* integrate FastGelu into test system for ONNX converters

* code review fixes

* returned constant calculation
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
…opset (apache#13119)

* add converter for FastGelu from Microsoft onnxruntime contrib opset

* integrate FastGelu into test system for ONNX converters

* code review fixes

* returned constant calculation
@KJlaccHoeUM9l KJlaccHoeUM9l deleted the agladyshev/gpt2/fast_gelu branch January 10, 2023 12:02
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.

5 participants