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

Fix issues of tanh(x) when x=INF or -INF #2786

Merged
merged 1 commit into from
Oct 27, 2023
Merged

Conversation

amdrexu
Copy link
Contributor

@amdrexu amdrexu commented Oct 26, 2023

We didn't check this special case. We just computed tanh(x) by sinh(x)/cosh(x). But when x=INF or -INF, the limit of tanh(x) is defined as follow:

lim(tanh(x)) = 1.0, x -> INF; lim(tanh(x)) = -1.0, x -> -INF

@amdrexu amdrexu requested a review from a team as a code owner October 26, 2023 09:19
@amdvlk-admin
Copy link

Test summary for commit 50d32aa

CTS tests (Failed: 0/137949)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35353/68947 (51.3%)
    • Failed: 0/68947 (0.0%)
    • Not Supported: 33594/68947 (48.7%)
    • Warnings: 0/68947 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35424/69002 (51.3%)
    • Failed: 0/69002 (0.0%)
    • Not Supported: 33578/69002 (48.7%)
    • Warnings: 0/69002 (0.0%)

Copy link
Member

@nhaehnle nhaehnle left a comment

Choose a reason for hiding this comment

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

I feel like I've had a discussion with @kezhaoAMD about this recently. But I think it was in an llpcfe context. Did you discuss with him? He had a solution using copysign which might be a little better.

@xazhangAMD
Copy link
Contributor

I feel like I've had a discussion with @kezhaoAMD about this recently. But I think it was in an llpcfe context. Did you discuss with him? He had a solution using copysign which might be a little better.

Yes, llpcfe uses a copysign.

  Value *FPOne = ConstantFP::get(call->getType(), 1);
  Value *isInf = m_builder->CreateIsInf(val);
  Value *infResult = m_builder->CreateBinaryIntrinsic(Intrinsic::copysign, FPOne, val);
  return m_builder->CreateSelect(isInf, infResult, tanh);

@amdrexu
Copy link
Contributor Author

amdrexu commented Oct 26, 2023

I feel like I've had a discussion with @kezhaoAMD about this recently. But I think it was in an llpcfe context. Did you discuss with him? He had a solution using copysign which might be a little better.

Yes, llpcfe uses a copysign.

  Value *FPOne = ConstantFP::get(call->getType(), 1);
  Value *isInf = m_builder->CreateIsInf(val);
  Value *infResult = m_builder->CreateBinaryIntrinsic(Intrinsic::copysign, FPOne, val);
  return m_builder->CreateSelect(isInf, infResult, tanh);

Thank you. Yes, this is better and saves a v_cndmask. I will follow it.

@xazhangAMD
Copy link
Contributor

I feel like I've had a discussion with @kezhaoAMD about this recently. But I think it was in an llpcfe context. Did you discuss with him? He had a solution using copysign which might be a little better.

Yes, llpcfe uses a copysign.

  Value *FPOne = ConstantFP::get(call->getType(), 1);
  Value *isInf = m_builder->CreateIsInf(val);
  Value *infResult = m_builder->CreateBinaryIntrinsic(Intrinsic::copysign, FPOne, val);
  return m_builder->CreateSelect(isInf, infResult, tanh);

Thank you. Yes, this is better and saves a v_cndmask. I will follow it.

I didn't add it into lgc because I was not sure whether GLSL spec requires this. I can drop the llpcfe approach when the lgc commit is merged.

We didn't check this special case. We just computed tanh(x) by
sinh(x)/cosh(x). But when x=INF or -INF, the limit of tanh(x) is defined
as follow:

  lim(tanh(x)) = 1.0, x -> INF; lim(tanh(x)) = -1.0, x -> -INF
@amdvlk-admin
Copy link

Test summary for commit db6e682

CTS tests (Failed: 0/137949)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35353/68947 (51.3%)
    • Failed: 0/68947 (0.0%)
    • Not Supported: 33594/68947 (48.7%)
    • Warnings: 0/68947 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35424/69002 (51.3%)
    • Failed: 0/69002 (0.0%)
    • Not Supported: 33578/69002 (48.7%)
    • Warnings: 0/69002 (0.0%)

@amdrexu
Copy link
Contributor Author

amdrexu commented Oct 27, 2023

I feel like I've had a discussion with @kezhaoAMD about this recently. But I think it was in an llpcfe context. Did you discuss with him? He had a solution using copysign which might be a little better.

Yes, llpcfe uses a copysign.

  Value *FPOne = ConstantFP::get(call->getType(), 1);
  Value *isInf = m_builder->CreateIsInf(val);
  Value *infResult = m_builder->CreateBinaryIntrinsic(Intrinsic::copysign, FPOne, val);
  return m_builder->CreateSelect(isInf, infResult, tanh);

Thank you. Yes, this is better and saves a v_cndmask. I will follow it.

I didn't add it into lgc because I was not sure whether GLSL spec requires this. I can drop the llpcfe approach when the lgc commit is merged.

Thank you. Actually, GLSL spec allows this but we didn't encounter such issues because there is no test.

Copy link
Member

@nhaehnle nhaehnle left a comment

Choose a reason for hiding this comment

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

Thanks!

@amdrexu amdrexu merged commit 1816e93 into GPUOpen-Drivers:dev Oct 27, 2023
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.

4 participants