Skip to content

[SYCL] Provide initialization for variable #11639

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
merged 1 commit into from
Oct 24, 2023

Conversation

frasercrmck
Copy link
Contributor

@frasercrmck frasercrmck commented Oct 24, 2023

As far as the compiler is concerned, the variable x is uninitialized on some control-flow paths and is passed to the function (ex|in)clusive_scan_over_group.

In terms of LLVM, the function parameter that x is passed to is likely marked noundef. This can lead the compiler to take advantage of undef rules and assume that the control path in which x is defined must happen (otherwise it'd be undefined), and it can essentially remove the guarding i < N around the memory accesses. This is, of course, dangerous.

Though, in practice, this is often avoided because the scan function is inlined before LLVM makes that assumption, this should not be relied upon. I've chosen to zero-initialize it. The value of init cannot be used because it may be out of bounds for the type pointed to by InPtr, which can introduce new UB into the program.

@frasercrmck frasercrmck requested a review from a team as a code owner October 24, 2023 14:08
As far as the compiler is concerned, the variable `x` is uninitialized
on some control-flow paths and is passed to the function
`(ex|in)clusive_scan_over_group`.

In terms of LLVM, the function parameter that `x` is passed to is likely
marked `noundef`. This can lead the compiler to take advantage of
`undef` rules and assume that the control path in which `x` is
defined must happen (otherwise it'd be undefined), and it can
essentially remove the guarding `i < N` around the memory accesses. This
is, of course, dangerous.

Though, in practice, this is often avoided because the scan function is
inlined before LLVM makes that assumption, this should not be relied
upon. I've chosen to zero-initialize it. The value of `init` cannot be
used because it may be out of bounds for the type pointed to by `InPtr`,
which can introduce new UB into the program.
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

Side note: Please avoid force-pushing to open PR branches. It erases history and might make it difficult for reviewers to follow along. Not a big deal for this PR though. 😄

@frasercrmck
Copy link
Contributor Author

LGTM! Thanks!

Side note: Please avoid force-pushing to open PR branches. It erases history and might make it difficult for reviewers to follow along. Not a big deal for this PR though. 😄

Oh right, yes I knew that, sorry!

@frasercrmck frasercrmck temporarily deployed to WindowsCILock October 24, 2023 14:47 — with GitHub Actions Inactive
@frasercrmck frasercrmck temporarily deployed to WindowsCILock October 24, 2023 15:07 — with GitHub Actions Inactive
@againull againull merged commit d4df692 into intel:sycl Oct 24, 2023
@frasercrmck frasercrmck deleted the fix-undef-variable branch October 24, 2023 17:09
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.

3 participants