Skip to content

Conversation

@tcreech-intel
Copy link
Contributor

It is possible for a function declaration to be used by both @llvm.used
and, e.g., a spir_func definition. Ensure that we don't attempt to erase
such declarations when removing @llvm.used.

It is possible for a function declaration to be used by both @llvm.used
and, e.g., a spir_func definition. Ensure that we don't attempt to erase
such declarations when removing @llvm.used.
@tcreech-intel tcreech-intel marked this pull request as ready for review June 14, 2022 19:58
@tcreech-intel tcreech-intel requested a review from a team as a code owner June 14, 2022 19:58
@tcreech-intel tcreech-intel requested a review from asudarsa June 14, 2022 19:59
asudarsa
asudarsa previously approved these changes Jun 14, 2022
Copy link
Contributor

@asudarsa asudarsa 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

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.

@tcreech-intel, could you add a regression test, please?

Most builds will hit an assertion in the first RUN line when the
declaration is incorrectly erased.
@tcreech-intel
Copy link
Contributor Author

@tcreech-intel, could you add a regression test, please?

Hi @bader -- absolutely. Done.

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.

Thanks!

@pvchupin pvchupin merged commit 9b4ed34 into intel:sycl Jun 15, 2022
@tcreech-intel tcreech-intel deleted the llvmused_and_spirfunc_used_func branch June 15, 2022 22:06
Copy link
Contributor

@asudarsa asudarsa 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

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