Skip to content

Conversation

@jinge90
Copy link
Contributor

@jinge90 jinge90 commented Jan 20, 2021

Signed-off-by: gejin [email protected]

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)

@jinge90
Copy link
Contributor Author

jinge90 commented Jan 20, 2021

/summary:run

@jinge90 jinge90 requested a review from bader January 21, 2021 07:11
@jinge90 jinge90 requested a review from bader January 21, 2021 07:42
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
iclsrc pushed a commit that referenced this pull request Mar 28, 2025
Update the pattern after llvm-project commit da0f9e7 ("Reland: [MC]
output inlined-at debug info (#106230) (#130306)", 2025-03-11).

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@4adbf65f5278346
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