Skip to content
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

pow() with a negative base fails on non-LLVM backends #5915

Closed
strongoier opened this issue Aug 30, 2022 · 0 comments
Closed

pow() with a negative base fails on non-LLVM backends #5915

strongoier opened this issue Aug 30, 2022 · 0 comments
Assignees
Labels
potential bug Something that looks like a bug but not yet confirmed
Milestone

Comments

@strongoier
Copy link
Contributor

When arch=ti.cpu/ti.cuda, the following script,

import taichi as ti

ti.init()

@ti.kernel
def calc_pow(i: ti.i32) -> ti.f32:
    return pow(-1.0, i)

print(calc_pow(1))

gives -1.0 , which is correct. However, it gives 1.0 when arch=ti.metal, and gives nan when arch=ti.opengl.

This is because the native pow(x, y) function in Metal Shading Language/GLSL is implemented with exp2(y * log2(x)), so it doesn't support negative bases. See https://developer.apple.com/metal/Metal-Shading-Language-Specification.pdf and https://registry.khronos.org/OpenGL/specs/gl/GLSLangSpec.4.50.pdf for more details.

The behavior is understandable, because pow() with a negative base and a real exponent results in a complex number. What we can do is to follow C++ - specially handle the case where the exponent is an integer, which results in a real number. In this way we can make the behavior of pow() consistent across all backends. I'll submit a PR for that.

@strongoier strongoier added the potential bug Something that looks like a bug but not yet confirmed label Aug 30, 2022
@taichi-gardener taichi-gardener moved this to Untriaged in Taichi Lang Aug 30, 2022
@strongoier strongoier added this to the v1.1.3 milestone Aug 30, 2022
@strongoier strongoier self-assigned this Sep 2, 2022
@strongoier strongoier moved this from Untriaged to Todo in Taichi Lang Sep 2, 2022
strongoier added a commit that referenced this issue Sep 8, 2022
I found that `abs(i64)` was not supported while fixing #5915. This PR
adds support for that.

<!--
Thank you for your contribution!

If it is your first time contributing to Taichi, please read our
Contributor Guidelines:
  https://docs.taichi-lang.org/docs/contributor_guide

- Please always prepend your PR title with tags such as [CUDA], [Lang],
[Doc], [Example]. For a complete list of valid PR tags, please check out
https://github.com/taichi-dev/taichi/blob/master/misc/prtags.json.
- Use upper-case tags (e.g., [Metal]) for PRs that change public APIs.
Otherwise, please use lower-case tags (e.g., [metal]).
- More details:
https://docs.taichi-lang.org/docs/contributor_guide#pr-title-format-and-tags

- Please fill in the issue number that this PR relates to.
- If your PR fixes the issue **completely**, use the `close` or `fixes`
prefix so that GitHub automatically closes the issue when the PR is
merged. For example,
    Related issue = close #2345
- If the PR does not belong to any existing issue, free to leave it
blank.
-->
strongoier added a commit that referenced this issue Sep 13, 2022
Related issue = #5915

Just as proposed in #5915, we specially handle `pow()` with an integer
exponent by demoting it with manual fast-pow implementation in the
`demote_operations` pass so that `pow()` can have the same behavior
across backends. In this case the return type is the type of base (lhs).

<!--
Thank you for your contribution!

If it is your first time contributing to Taichi, please read our
Contributor Guidelines:
  https://docs.taichi-lang.org/docs/contributor_guide

- Please always prepend your PR title with tags such as [CUDA], [Lang],
[Doc], [Example]. For a complete list of valid PR tags, please check out
https://github.com/taichi-dev/taichi/blob/master/misc/prtags.json.
- Use upper-case tags (e.g., [Metal]) for PRs that change public APIs.
Otherwise, please use lower-case tags (e.g., [metal]).
- More details:
https://docs.taichi-lang.org/docs/contributor_guide#pr-title-format-and-tags

- Please fill in the issue number that this PR relates to.
- If your PR fixes the issue **completely**, use the `close` or `fixes`
prefix so that GitHub automatically closes the issue when the PR is
merged. For example,
    Related issue = close #2345
- If the PR does not belong to any existing issue, free to leave it
blank.
-->

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
strongoier added a commit that referenced this issue Sep 13, 2022
Related issue = #5915

This PR resolves
#6044 (comment).

<!--
Thank you for your contribution!

If it is your first time contributing to Taichi, please read our
Contributor Guidelines:
  https://docs.taichi-lang.org/docs/contributor_guide

- Please always prepend your PR title with tags such as [CUDA], [Lang],
[Doc], [Example]. For a complete list of valid PR tags, please check out
https://github.com/taichi-dev/taichi/blob/master/misc/prtags.json.
- Use upper-case tags (e.g., [Metal]) for PRs that change public APIs.
Otherwise, please use lower-case tags (e.g., [metal]).
- More details:
https://docs.taichi-lang.org/docs/contributor_guide#pr-title-format-and-tags

- Please fill in the issue number that this PR relates to.
- If your PR fixes the issue **completely**, use the `close` or `fixes`
prefix so that GitHub automatically closes the issue when the PR is
merged. For example,
    Related issue = close #2345
- If the PR does not belong to any existing issue, free to leave it
blank.
-->

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
ailzhang pushed a commit that referenced this issue Sep 14, 2022
Related issue = #5915

As `pow()` with integer exponent is already demoted (#6044), there's no
need for backends to handle integer pows.
Repository owner moved this from Todo to Done in Taichi Lang Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
potential bug Something that looks like a bug but not yet confirmed
Projects
Status: Done
Development

No branches or pull requests

1 participant