[FPGA][SYCL] Add support for memory attributes on device_global variables #12785

smanna12 · 2024-02-21T16:31:39Z

Currently we allow FPGA memory attributes on const variables. This patch relaxes the restriction that permits the attributes when applied on non-const device_global variables and updates the error message to properly convey which types of variables are allowed to have attributes. Also this patch relaxes the restriction that permits the private_copies attribute when applied on const variables. Previously we had restriction that the private_copies attribute would work with local non-const variables only.

…bles Signed-off-by: Soumi Manna <soumi.manna@intel.com>

clang/include/clang/Basic/Attr.td

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/lib/Sema/SemaDeclAttr.cpp

clang/test/CodeGenSYCL/device_global.cpp

clang/test/SemaSYCL/intel-fpga-device-global.cpp

smanna12 · 2024-02-27T05:41:59Z

Thanks @premanandrao for reviews and feedbacks!

premanandrao · 2024-02-29T16:30:30Z

@premanandrao, I checked with @artemrad about this. The ask, was always to allow all FPGA memory attributes on non-const device_globals. The scope has not changed.

Okay, I am fine if that is the case. We just need to make sure that only the exact restrictions are called for each attribute.

I could not able to add support for non const global_variables without refactoring common codes since isTypeDecoratedWithDeclAttribute() is available in sema. Also while working on this, i notice that we have few test coverages for memory attributes when they apply for const/local/static etc. So This PR ends up adding more tests cases to make sure I do not create any regression.

Sounds good. I will review again after the restriction difference is fixed.

smanna12 · 2024-02-29T18:22:21Z

@premanandrao, I checked with @artemrad about this. The ask, was always to allow all FPGA memory attributes on non-const device_globals. The scope has not changed.

Okay, I am fine if that is the case. We just need to make sure that only the exact restrictions are called for each attribute.

I could not able to add support for non const global_variables without refactoring common codes since isTypeDecoratedWithDeclAttribute() is available in sema. Also while working on this, i notice that we have few test coverages for memory attributes when they apply for const/local/static etc. So This PR ends up adding more tests cases to make sure I do not create any regression.

Sounds good. I will review again after the restriction difference is fixed.

@premanandrao, I have updated patch to fix restriction difference (a314482). Does this address your concerns? The new commit makes sure that only the exact restrictions are called for each attribute with addition of non-const device global.

clang/test/SemaSYCL/intel-fpga-local.cpp

clang/lib/Sema/SemaDeclAttr.cpp

clang/include/clang/Basic/Attr.td

clang/lib/Sema/SemaDeclAttr.cpp

premanandrao

LGTM @smanna12

smanna12 · 2024-03-01T22:25:15Z

Thank you @premanandrao for the reviews!

smanna12 · 2024-03-02T02:42:27Z

latest commit was just format fixes (4a4a016).

@intel/llvm-gatekeepers, This PR is ready. Could you please merge this PR? This feature is very important for FPGA team.

…evice compilation (#12930) #12785 added support for non-const device_global variables on all FPGA memory attributes. FPGA memory attributes should only work for device codes. They should have no effect on host code. This patch fixes bug where we emitted error when attributes are applied to device_globals variables for host compilation. --------- Signed-off-by: Soumi Manna <soumi.manna@intel.com>

[FPGA][SYCL] Allow memory attributes on non-const device_global varia…

38dc2f5

…bles Signed-off-by: Soumi Manna <soumi.manna@intel.com>

smanna12 changed the title ~~[FPGA][SYCL] Allow memory attributes on non-const device_global varia…~~ [FPGA][SYCL] Allow memory attributes on non-const device_global variables Feb 21, 2024

smanna12 had a problem deploying to WindowsCILock February 21, 2024 16:51 — with GitHub Actions Failure

smanna12 requested a review from premanandrao February 21, 2024 16:54

premanandrao reviewed Feb 21, 2024

View reviewed changes

clang/include/clang/Basic/Attr.td Outdated Show resolved Hide resolved

smanna12 had a problem deploying to WindowsCILock February 21, 2024 17:53 — with GitHub Actions Failure

Add support for device_global variables on FPGA attributes

db826f5

smanna12 had a problem deploying to WindowsCILock February 26, 2024 06:33 — with GitHub Actions Error

Update patch

5e573c1

smanna12 temporarily deployed to WindowsCILock February 26, 2024 06:40 — with GitHub Actions Inactive

smanna12 had a problem deploying to WindowsCILock February 26, 2024 07:13 — with GitHub Actions Failure

premanandrao reviewed Feb 26, 2024

View reviewed changes

Address Review Comments

e45c0b5

smanna12 had a problem deploying to WindowsCILock February 27, 2024 05:37 — with GitHub Actions Error

Update tests

6eb22a0

smanna12 temporarily deployed to WindowsCILock February 27, 2024 05:41 — with GitHub Actions Inactive

smanna12 changed the title ~~[FPGA][SYCL] Allow memory attributes on non-const device_global variables~~ [FPGA][SYCL] Allow memory attributes on device_global variables Feb 27, 2024

smanna12 changed the title ~~[FPGA][SYCL] Allow memory attributes on device_global variables~~ [FPGA][SYCL] Allow memory attributes on non-const device_global variables Feb 27, 2024

smanna12 temporarily deployed to WindowsCILock February 27, 2024 06:15 — with GitHub Actions Inactive

Allow max_replicates attribute on device_global

21d5362

smanna12 temporarily deployed to WindowsCILock February 27, 2024 07:06 — with GitHub Actions Inactive

smanna12 had a problem deploying to WindowsCILock February 27, 2024 07:42 — with GitHub Actions Failure

Remove duplicate codes

5f3ea15

smanna12 temporarily deployed to WindowsCILock February 27, 2024 14:38 — with GitHub Actions Inactive

smanna12 changed the title ~~[FPGA][SYCL] Allow memory attributes on non-const device_global variables~~ [FPGA][SYCL] Add support for memory attributes on device_global variables Feb 27, 2024

smanna12 had a problem deploying to WindowsCILock February 27, 2024 15:06 — with GitHub Actions Error

Add support for force_pow2_depth attribute on device_global variable.

3dad7b2

smanna12 temporarily deployed to WindowsCILock February 27, 2024 15:25 — with GitHub Actions Inactive

smanna12 temporarily deployed to WindowsCILock February 27, 2024 15:53 — with GitHub Actions Inactive

smanna12 temporarily deployed to WindowsCILock February 29, 2024 17:21 — with GitHub Actions Inactive

smanna12 had a problem deploying to WindowsCILock February 29, 2024 18:07 — with GitHub Actions Failure

Fix existing attributes restrictions

a314482

Merge remote-tracking branch 'my_remote/sycl' into FixFPGABug

48a15fa

smanna12 temporarily deployed to WindowsCILock February 29, 2024 19:42 — with GitHub Actions Inactive

smanna12 commented Feb 29, 2024

View reviewed changes

clang/test/SemaSYCL/intel-fpga-local.cpp Show resolved Hide resolved

smanna12 had a problem deploying to WindowsCILock February 29, 2024 20:24 — with GitHub Actions Failure

smanna12 temporarily deployed to WindowsCILock March 1, 2024 00:40 — with GitHub Actions Inactive

smanna12 temporarily deployed to WindowsCILock March 1, 2024 00:41 — with GitHub Actions Inactive

smanna12 requested a review from premanandrao March 1, 2024 03:00

premanandrao reviewed Mar 1, 2024

View reviewed changes

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

clang/include/clang/Basic/Attr.td Show resolved Hide resolved

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

Address review comments

32ae573

smanna12 temporarily deployed to WindowsCILock March 1, 2024 22:10 — with GitHub Actions Inactive

smanna12 marked this pull request as ready for review March 1, 2024 22:16

smanna12 requested a review from a team as a code owner March 1, 2024 22:16

smanna12 requested a review from premanandrao March 1, 2024 22:19

premanandrao approved these changes Mar 1, 2024

View reviewed changes

smanna12 had a problem deploying to WindowsCILock March 1, 2024 22:38 — with GitHub Actions Failure

Fix clang format errors

4a4a016

smanna12 temporarily deployed to WindowsCILock March 1, 2024 23:46 — with GitHub Actions Inactive

smanna12 temporarily deployed to WindowsCILock March 2, 2024 00:13 — with GitHub Actions Inactive

ldrumm merged commit 3fc6708 into intel:sycl Mar 4, 2024
12 checks passed

smanna12 deleted the FixFPGABug branch March 4, 2024 16:04

smanna12 mentioned this pull request Mar 6, 2024

[SYCL][FPGA] Allow memory attributes on device_global variables for device compilation #12930

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FPGA][SYCL] Add support for memory attributes on device_global variables #12785

[FPGA][SYCL] Add support for memory attributes on device_global variables #12785

smanna12 commented Feb 21, 2024 •

edited

Loading

smanna12 commented Feb 27, 2024

premanandrao commented Feb 29, 2024

smanna12 commented Feb 29, 2024 •

edited

Loading

premanandrao left a comment

smanna12 commented Mar 1, 2024

smanna12 commented Mar 2, 2024 •

edited

Loading

[FPGA][SYCL] Add support for memory attributes on device_global variables #12785

[FPGA][SYCL] Add support for memory attributes on device_global variables #12785

Conversation

smanna12 commented Feb 21, 2024 • edited Loading

smanna12 commented Feb 27, 2024

premanandrao commented Feb 29, 2024

smanna12 commented Feb 29, 2024 • edited Loading

premanandrao left a comment

Choose a reason for hiding this comment

smanna12 commented Mar 1, 2024

smanna12 commented Mar 2, 2024 • edited Loading

smanna12 commented Feb 21, 2024 •

edited

Loading

smanna12 commented Feb 29, 2024 •

edited

Loading

smanna12 commented Mar 2, 2024 •

edited

Loading