Skip to content

[SYCL] Add support for __arithmetic_fence builtin for SYCL targets. #8072

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

Closed
wants to merge 3 commits into from

Conversation

zahiraam
Copy link
Contributor

@zahiraam zahiraam commented Jan 20, 2023

__arithmetic_fence enforces ordering on expression evaluation when fast-math is enabled.
In fast math mode some floating-point optimizations are performed such as reassociation and distribution.

For example, the compiler may transform (a+b)+c into a+(b+c). Although these two expressions are equivalent in integer arithmetic, they may not be in floating-point arithmetic. The builtin tells the compiler that the expression in parenthesis can’t be re-associated or distributed.
__arithmetic_fence(a+b)+c is not equivalent to a+(b+c).

@zahiraam zahiraam temporarily deployed to aws January 20, 2023 21:23 — with GitHub Actions Inactive
@zahiraam zahiraam temporarily deployed to aws January 20, 2023 21:54 — with GitHub Actions Inactive
@zahiraam zahiraam marked this pull request as ready for review January 23, 2023 16:05
@zahiraam zahiraam requested a review from a team as a code owner January 23, 2023 16:05
@Fznamznon Fznamznon changed the title Add support for __arithemetic_fence builtin for SYCL targets. Add support for __arithmetic_fence builtin for SYCL targets. Jan 23, 2023
@zahiraam zahiraam temporarily deployed to aws January 23, 2023 17:45 — with GitHub Actions Inactive
@zahiraam zahiraam temporarily deployed to aws January 23, 2023 18:24 — with GitHub Actions Inactive
@zahiraam zahiraam requested a review from Fznamznon January 23, 2023 20:01
Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

One NIT, otherwise lgtm

@zahiraam zahiraam temporarily deployed to aws January 24, 2023 14:22 — with GitHub Actions Inactive
@zahiraam zahiraam temporarily deployed to aws January 24, 2023 17:17 — with GitHub Actions Inactive
@zahiraam
Copy link
Contributor Author

@intel/llvm-gatekeepers I don't think the failure is related to this PR. In which case the PR is ready to merge. Thanks.

Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

Is there a reason we are doing this in intel/llvm and not community?

@zahiraam
Copy link
Contributor Author

Is there a reason we are doing this in intel/llvm and not community?

I can't think of any reason.

@elizabethandrews
Copy link
Contributor

Is there a reason we are doing this in intel/llvm and not community?

I can't think of any reason.

I would suggest attempting to put this in community instead then. We always try to do that if possible.

@zahiraam
Copy link
Contributor Author

Is there a reason we are doing this in intel/llvm and not community?

I can't think of any reason.

I would suggest attempting to put this in community instead then. We always try to do that if possible.

I just verified and sycl_device attribute is not implemented.

@bader
Copy link
Contributor

bader commented Jan 24, 2023

Is there a reason we are doing this in intel/llvm and not community?

I can't think of any reason.

I would suggest attempting to put this in community instead then. We always try to do that if possible.

I just verified and sycl_device attribute is not implemented.

sycl_kernel is supported.

@bader
Copy link
Contributor

bader commented Jan 24, 2023

@zahiraam, could you add tag(s) for the PR title, please? https://github.com/intel/llvm/blob/sycl/CONTRIBUTING.md#pull-request

Add support for __arithmetic_fence builtin for SYCL targets.

SYCL or SPIR?

@zahiraam zahiraam changed the title Add support for __arithmetic_fence builtin for SYCL targets. [SYCL] Add support for __arithmetic_fence builtin for SYCL targets. Jan 24, 2023
@bader
Copy link
Contributor

bader commented Jan 25, 2023

I suggest following @elizabethandrews suggestion and commit this change to llvm.org.

A couple of notes:

  • I don't think this change is SYCL specific and we need to replace SYCL with SPIR in the log message.
  • Test doesn't need to use SYCL mode as well. You can keep using C++, just add compilation for the SPIR target.
    • If you'd like to check this feature in SYCL mode, please, add new SYCL specific tests in CodeGenSYCL and SemaSYCL directories. You can use existing SYCL tests as a reference.

@zahiraam zahiraam closed this Jan 26, 2023
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.

6 participants