-
Notifications
You must be signed in to change notification settings - Fork 787
[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
[SYCL][Driver] Force precise division rounding for precise model #12107
Conversation
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]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK for driver.
There was a problem hiding this 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 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. |
@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. |
…del (intel#12107)" This reverts commit bfbf8ab.
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.