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 signed zero issues of FRem #2782

Closed
wants to merge 1 commit into from
Closed

Conversation

amdrexu
Copy link
Contributor

@amdrexu amdrexu commented Oct 25, 2023

We use IR builder CreateFRem to translate OpFRem to LLVM native frem instruction. The instruction is further lowered by backend following such formula:

frem(x, y) = x - y * trunc(x/y)

There is a latent issue when x=-0.0. According to SPIR-V spec, the sign of the result of FRem is the same as the sign of the dividend. But when we input x=-0.0 to above formula, we finally get the addition of (-0.0) + 0.0. The result is +0.0 returned by HW. Hence, we have to manually check this special case when nsz fast math flag is not specified.

@amdrexu amdrexu requested a review from a team as a code owner October 25, 2023 06:32
@amdvlk-admin
Copy link

Test summary for commit fc12f6a

CTS tests (Failed: 0/138994)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 41144/69503 (59.2%)
    • Failed: 0/69503 (0.0%)
    • Not Supported: 28359/69503 (40.8%)
    • Warnings: 0/69503 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 41177/69491 (59.3%)
    • Failed: 0/69491 (0.0%)
    • Not Supported: 28314/69491 (40.7%)
    • Warnings: 0/69491 (0.0%)

We use IR builder CreateFRem to translate OpFRem to LLVM native frem
instruction. The instruction is further lowered by backend following
such formula:

  frem(x, y) = x - y * trunc(x/y)

There is a latent issue when x=-0.0. According to SPIR-V spec, the sign
of the result of FRem is the same as the sign of the dividend. But when
we input x=-0.0 to above formula, we finally get the addition of
(-0.0) + 0.0. The result is +0.0 returned by HW. Hence, we have to
manually check this special case when nsz fast math flag is not specified.
@amdvlk-admin
Copy link

Test summary for commit efec18e

CTS tests (Failed: 0/137949)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35352/68947 (51.3%)
    • Failed: 0/68947 (0.0%)
    • Not Supported: 33595/68947 (48.7%)
    • Warnings: 0/68947 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35423/69002 (51.3%)
    • Failed: 0/69002 (0.0%)
    • Not Supported: 33579/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 just looked into LLVM LangRef.rst. It says this about frem:

The value produced is the floating-point remainder of the two operands. This is the same output as a libm 'fmod' function, but without any possibility of setting errno. The remainder has the same sign as the dividend.

It seems like we should probably consider this an LLVM backend bug that must be fixed there instead of in LLPC?

cc @kezhaoAMD This might affect some of your stuff as well.

@amdrexu
Copy link
Contributor Author

amdrexu commented Oct 26, 2023

I just looked into LLVM LangRef.rst. It says this about frem:

The value produced is the floating-point remainder of the two operands. This is the same output as a libm 'fmod' function, but without any possibility of setting errno. The remainder has the same sign as the dividend.

It seems like we should probably consider this an LLVM backend bug that must be fixed there instead of in LLPC?

cc @kezhaoAMD This might affect some of your stuff as well.

I created an issue ticket to LLVM backend team. If this could be resolved quickly, I will drop this change. Otherwise, I will keep it as a temporary workaround and remove it once backend fix lands.

@amdrexu
Copy link
Contributor Author

amdrexu commented Oct 27, 2023

Thanks @jayfoad. I close this PR since a better fix is in LLVM itself: llvm/llvm-project#70448

@amdrexu amdrexu closed this Oct 27, 2023
@jayfoad
Copy link
Member

jayfoad commented Oct 31, 2023

According to SPIR-V spec, the sign of the result of FRem is the same as the sign of the dividend. But when we input x=-0.0 to above formula, we finally get the addition of (-0.0) + 0.0. The result is +0.0 returned by HW.

The SPIR-V spec (https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpFRem) only specifies the sign of the result when the result is not zero, so this case is OK.

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