[SYCL] Add clang implementation for accessor property no_alias #3452

schittir · 2021-03-31T07:33:37Z

no_alias is a compile-time-constant accessor property.
It exists in cl::sycl::ONEAPI::property scope.
When applied the LLVM IR should contain the noalias parameter attribute
on the kernel argument corresponding to that accessor.
This patch uses clang's RestrictAttr to emit noalias i.e., llvm::Attribute::NoAlias.

AaronBallman

Can you also remove [WIP] from the title and add [SYCL] instead?

clang/lib/CodeGen/CodeGenModule.cpp

clang/lib/Sema/SemaSYCL.cpp

schittir · 2021-04-03T18:01:30Z

I'm not sure why the build bot lit test runs fail. They all pass on my local workspace.

AaronBallman · 2021-04-05T12:24:54Z

I'm not sure why the build bot lit test runs fail. They all pass on my local workspace.

You should try catching the PR up to upstream. The testing issue may have been resolved elsewhere, or it'll restart CI in case the issues are transient.

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

3. replace variable names with string literals

schittir · 2021-04-06T17:09:01Z

I'm not sure why the build bot lit test runs fail. They all pass on my local workspace.

You should try catching the PR up to upstream. The testing issue may have been resolved elsewhere, or it'll restart CI in case the issues are transient.

Caught up PR to upstream and resolved failing tests.

clang/test/CodeGenSYCL/disable_loop_pipelining.cpp

clang/test/CodeGenSYCL/initiation_interval.cpp

MrSidims

Overall LGTM

clang/lib/CodeGen/CodeGenModule.cpp

MrSidims

Wait a second. Wasn't no_alias property supposed to generate function parameter attribute, instead of a metadata?

clang/lib/Sema/SemaSYCL.cpp

Update test

schittir · 2021-04-07T00:02:58Z

Wait a second. Wasn't no_alias property supposed to generate function parameter attribute, instead of a metadata?

Thanks for the catch. Changed it to generate a function parameter attribute instead of metadata.

MrSidims

LGTM, but you shall change the description.

clang/test/CodeGenSYCL/disable_loop_pipelining.cpp

premanandrao

LGTM, but please check the pre-commit run-time failure.

smanna12 · 2021-04-07T15:22:35Z

LGTM. I looked at the pre-commit failure:

http://icl-jenkins.sc.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FLin%2FLLVM_Test_Suite/detail/LLVM_Test_Suite/3272/pipeline

Failed Tests (1):
SYCL :: Reduction/reduction_nd_N_vars.cpp

@schittir, you can try restarting jenkins/precommit testing.

bader · 2021-04-07T15:38:02Z

@schittir, you can try restarting jenkins/precommit testing.

This should not be the solution for pre-commit failures. The test must pass w/o re-starts. Please, investigate why the test gives wrong results.
Tagging @v-klochkov for awareness.

schittir · 2021-04-07T15:46:35Z

LGTM, but you shall change the description.

Done.

clang/test/CodeGenSYCL/accessor_no_alias_property.cpp

smanna12

LGTM

clang/lib/Sema/SemaSYCL.cpp

AaronBallman

LGTM!

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2021-08-06T17:50:06Z

clang/test/CodeGenSYCL/accessor_no_alias_property.cpp

+
+// check that noalias parameter attribute is NOT emitted when it is not used
+// CHECK: define {{.*}}spir_kernel void @_ZTSZ4mainE16kernel_function2{{.*}} !kernel_arg_buffer_location
+// CHECK-NOT: define {{.*}}spir_kernel void @_ZTSZ4mainE16kernel_function2({{.*}} noalias {{.*}}


Lines 6 and 7 here are broken. The first checks to see if that kernel is defined, and the 2nd just checks to see if it is defined AGAIN. This is not correct. Presumably there needs to be a 'CHECK-NOT'/'CHECK-SAME' combo, like in the example here:
https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-same-directive

schittir force-pushed the accessor_no_alias_property branch from 178f9e7 to e3dbe8b Compare April 1, 2021 17:32

schittir requested review from AaronBallman and elizabethandrews April 1, 2021 18:07

schittir marked this pull request as ready for review April 1, 2021 18:07

schittir requested a review from premanandrao as a code owner April 1, 2021 18:07

AaronBallman reviewed Apr 1, 2021

View reviewed changes

schittir changed the title ~~[WIP] Add clang implementation for accessor property no_alias~~ Add clang implementation for accessor property no_alias Apr 1, 2021

schittir changed the title ~~Add clang implementation for accessor property no_alias~~ [SYCL] Add clang implementation for accessor property no_alias Apr 1, 2021

schittir and others added 7 commits April 5, 2021 13:23

25700: Add clang implementation for accessor property no_alias

d7e3e04

WIP edit 2

0b55f90

Update clang/lib/CodeGen/CodeGenModule.cpp

c7441a1

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

Update clang/lib/CodeGen/CodeGenModule.cpp

5b3446d

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

Update clang/lib/Sema/SemaSYCL.cpp

57ca989

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

Update clang/lib/Sema/SemaSYCL.cpp

9bbe8f9

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

1. Fix format 2. replace "llvm::Attribute::NoAlias with "noalias"

c38d9f4

3. replace variable names with string literals

schittir force-pushed the accessor_no_alias_property branch from 89e735a to c38d9f4 Compare April 5, 2021 20:25

schittir added 2 commits April 5, 2021 13:42

Fix formatting again

75b3fb1

Fix failing lit tests

d2642be

smanna12 reviewed Apr 6, 2021

View reviewed changes

clang/test/CodeGenSYCL/disable_loop_pipelining.cpp Outdated Show resolved Hide resolved

smanna12 reviewed Apr 6, 2021

View reviewed changes

clang/test/CodeGenSYCL/initiation_interval.cpp Outdated Show resolved Hide resolved

MrSidims reviewed Apr 6, 2021

View reviewed changes

clang/lib/CodeGen/CodeGenModule.cpp Outdated Show resolved Hide resolved

MrSidims requested changes Apr 6, 2021

View reviewed changes

MrSidims reviewed Apr 6, 2021

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

Change noalias to be a parameter attribute, not a function metadata

ca91261

Update test

schittir requested review from AaronBallman and MrSidims April 7, 2021 00:04

schittir requested a review from smanna12 April 7, 2021 01:13

MrSidims reviewed Apr 7, 2021

View reviewed changes

clang/test/CodeGenSYCL/disable_loop_pipelining.cpp Outdated Show resolved Hide resolved

premanandrao reviewed Apr 7, 2021

View reviewed changes

Undo changes to lit tests

8c94561

MrSidims previously approved these changes Apr 7, 2021

View reviewed changes

schittir requested a review from premanandrao April 7, 2021 16:39

premanandrao reviewed Apr 7, 2021

View reviewed changes

clang/test/CodeGenSYCL/accessor_no_alias_property.cpp Show resolved Hide resolved

clang/test/CodeGenSYCL/accessor_no_alias_property.cpp Show resolved Hide resolved

Expand test case and add comments

7dd04ea

schittir dismissed MrSidims’s stale review via 7dd04ea April 7, 2021 18:33

premanandrao previously approved these changes Apr 7, 2021

View reviewed changes

smanna12 previously approved these changes Apr 7, 2021

View reviewed changes

bader reviewed Apr 8, 2021

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved

Merge branch 'sycl' into accessor_no_alias_property

067dae8

bader dismissed stale reviews from smanna12 and premanandrao via 067dae8 April 9, 2021 13:22

AaronBallman approved these changes Apr 9, 2021

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved

bader merged commit 9e2f92e into intel:sycl Apr 9, 2021

schittir mentioned this pull request Apr 10, 2021

Remove FE dependency on SYCL header scope of accessor properties #3529

Closed

erichkeane reviewed Aug 6, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Add clang implementation for accessor property no_alias #3452

[SYCL] Add clang implementation for accessor property no_alias #3452

schittir commented Mar 31, 2021 •

edited

Loading

AaronBallman left a comment

schittir commented Apr 3, 2021

AaronBallman commented Apr 5, 2021

schittir commented Apr 6, 2021

MrSidims left a comment

MrSidims left a comment

schittir commented Apr 7, 2021

MrSidims left a comment

premanandrao left a comment

smanna12 commented Apr 7, 2021 •

edited

Loading

bader commented Apr 7, 2021

schittir commented Apr 7, 2021

smanna12 left a comment

AaronBallman left a comment

erichkeane Aug 6, 2021

elizabethandrews Aug 6, 2021

[SYCL] Add clang implementation for accessor property no_alias #3452

[SYCL] Add clang implementation for accessor property no_alias #3452

Conversation

schittir commented Mar 31, 2021 • edited Loading

AaronBallman left a comment

Choose a reason for hiding this comment

schittir commented Apr 3, 2021

AaronBallman commented Apr 5, 2021

schittir commented Apr 6, 2021

MrSidims left a comment

Choose a reason for hiding this comment

MrSidims left a comment

Choose a reason for hiding this comment

schittir commented Apr 7, 2021

MrSidims left a comment

Choose a reason for hiding this comment

premanandrao left a comment

Choose a reason for hiding this comment

smanna12 commented Apr 7, 2021 • edited Loading

bader commented Apr 7, 2021

schittir commented Apr 7, 2021

smanna12 left a comment

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

erichkeane Aug 6, 2021

Choose a reason for hiding this comment

elizabethandrews Aug 6, 2021

Choose a reason for hiding this comment

schittir commented Mar 31, 2021 •

edited

Loading

smanna12 commented Apr 7, 2021 •

edited

Loading