Skip to content

[SYCL][Driver] Force precise division rounding for precise model #12107

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

Merged

Conversation

steffenlarsen
Copy link
Contributor

This commit makes the driver pass
-cl-fp32-correctly-rounded-divide-sqrt as backend compiler options for SPIR-V targets when the -fpp-model=precise option is used.

This commit makes the driver pass
`-cl-fp32-correctly-rounded-divide-sqrt` as backend compiler options
for SPIR-V targets when the `-fpp-model=precise` option is used.

Signed-off-by: Larsen, Steffen <[email protected]>
Copy link
Contributor

@mdtoguchi mdtoguchi left a comment

Choose a reason for hiding this comment

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

OK for driver.

Copy link
Contributor

@KseniyaTikhomirova KseniyaTikhomirova left a comment

Choose a reason for hiding this comment

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

SYCL RT part (sycl/test-e2e/Basic/float_division_precise.cpp) LGTM

@steffenlarsen steffenlarsen merged commit bfbf8ab into intel:sycl Dec 8, 2023
@coldav
Copy link
Contributor

coldav commented Dec 12, 2023

@steffenlarsen This is has caused a lot of failures for us in SYCL CTS for the oneAPI Construction Kit as we don't support this flag in our OpenCL driver. It is also optional per device https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_API.html#math-intrinsics-options. We get that in precise mode, when this flag is supported, you want to pass it to get more accurate results, but has it been decided that this is a new requirement for precise mode, that precise mode no longer supports implementations that lack this flag? SYCL-CTS enables precise mode because the default mode is not sufficiently precise to pass SYCL-CTS, but this flag is providing more accuracy than SYCL-CTS needs, and there appears to no longer be a way to be precise enough for SYCL-CTS, yet still support all OpenCL implementations.

Is this intentional? We need to know so that we know whether we will need to adapt our implementation.

Including @bader as I think there is a general issue here.

@steffenlarsen
Copy link
Contributor Author

@coldav - Unlike math builtins, the SYCL specification does not specify an allowed ULP for division, while OpenCL does. I discussed this with @gmlueck and a handful other people and the best solution seemed to be to add this flag. Whether or not this issue was shown by the current CTS depends on the implementation and I agree a lot of implementations were lucky enough to avoid it previously.

I was not aware of the restriction that the option was optional. I will see what we can do for the cases where they aren't available, but keep in mind that unless there are changes to the specification there could be future testing in the CTS that checks for results from floating point division, either directly or indirectly, so even if we can make a case for ignoring it if the device doesn't support it, said device might not be able to be conformant.

steffenlarsen added a commit to steffenlarsen/llvm that referenced this pull request Dec 14, 2023
steffenlarsen added a commit that referenced this pull request Dec 14, 2023
…del (#12173)

This commit reverts #12107 and the
follow-up patch due to unforeseen consequences of the new flag.
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