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

[SYCL] Enable some math builtins in SemaSYCL #3060

Merged
merged 3 commits into from
Jan 22, 2021

Conversation

jinge90
Copy link
Contributor

@jinge90 jinge90 commented Jan 20, 2021

Signed-off-by: gejin ge.jin@intel.com

This PR aims to enable some math builtin in SemaSYCL. We supported some llvm math intrinsic in this PR:
KhronosGroup/SPIRV-LLVM-Translator#855
So a bunch of math builtin which depend on those llvm math intrinsic can work now. This PR enables following math builtin:
fmax/f (depends on llvm.maxnum)
fmin/f (depends on llvm.minnum)
isinf (depends on llvm.fabs)
isfinite (depends on llvm.fabs)
isnormal (depends on llvm.fabs)
fpclassify (depends on llvm.fabs)

Signed-off-by: gejin <ge.jin@intel.com>
@jinge90
Copy link
Contributor Author

jinge90 commented Jan 20, 2021

/summary:run

Signed-off-by: gejin <ge.jin@intel.com>
@jinge90 jinge90 requested a review from bader January 21, 2021 07:11
@jinge90 jinge90 requested a review from bader January 21, 2021 07:42
Signed-off-by: gejin <ge.jin@intel.com>
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

LGTM, although I would change the title of the PR.
"[SYCL] Enable some math builtins in SemaSYCL" - most of the enabled built-ins are not from "math" category.

@keryell
Copy link
Contributor

keryell commented Jan 21, 2021

@Ralender would this solve some crashes we have on math stuff?

@bader bader merged commit 1040b94 into intel:sycl Jan 22, 2021
@jinge90
Copy link
Contributor Author

jinge90 commented Jan 25, 2021

@Ralender would this solve some crashes we have on math stuff?

Hi, @keryell
I think this PR will not fix any "crash" issue. Previously, those math builtin are forbidden in FE, if developers use them in SYCL device code, compiler will report compiling error in device compilation phase. Now, those math builtin are permitted which means device compiler will convert them to corresponding llvm intrinsic and no compiling error will be reported.
The reason why we disabled some math builtin is they will be converted to corresponding llvm intrinsic which can't be handled by llvm-spirv translator. Handling such llvm intrinsic will lead to llvm-spirv crash.
Before submitting this patch, we have submitted a patch to enable a bunch of llvm intrinsic in llvm-spirv which can solve some "crash" in llvm-spirv side: KhronosGroup/SPIRV-LLVM-Translator#855
Thanks very much.

@keryell
Copy link
Contributor

keryell commented Jan 26, 2021

I was not thinking to Intel related crashes but to Xilinx related crashes.
We are not using SPIR-V but another path which has some creative/unexpected/... effects on the Xilinx FPGA backend.

@jinge90 jinge90 deleted the support-builtins-inf-finite branch January 26, 2021 01:19
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