Skip to content

[SYCL][FPGA] Remove deprecated attribute functionality #4532

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 15, 2021
Merged

[SYCL][FPGA] Remove deprecated attribute functionality #4532

merged 4 commits into from
Sep 15, 2021

Conversation

smanna12
Copy link
Contributor

@smanna12 smanna12 commented Sep 9, 2021 •
edited
Loading

Several FPGA specific attributes in the intelfpga vendor namespace were deprecated in favor of a name in the intel vendor namespace on PR #2568.

Specifically, we deprecated the
[[intelfpga::attr]] spellings in favor of [[intel::fpga_attr]],
[[intelfpga::register]] spelling in favor of [[intel::fpga_register]], and
[[intelfpga::memory]] spelling in favor of [[intel::fpga_memory]].

This patch

  1. removes the deprecated functionality for the intelfpga:: spellings.
    The affected attributes are:
    intelfpga::num_simd_work_items
    intelfpga::max_work_group_size
    intelfpga::max_global_work_dim
    intelfpga::no_global_work_offset
    intelfpga::ivdep
    intelfpga::ii
    intelfpga::max_concurrency
    intelfpga::loop_coalesce
    intelfpga::disable_loop_pipelining
    intelfpga::max_interleaving
    intelfpga::speculated_iterations
    intelfpga::doublepump
    intelfpga::singlepump
    intelfpga::memory
    intelfpga::register
    intelfpga::bankwidth
    intelfpga::numbanks
    intelfpga::private_copies
    intelfpga::merge
    intelfpga::max_replicates
    intelfpga::simple_dual_port
    intelfpga::bank_bits
    intelfpga::force_pow2_depth
    intelfpga::scheduler_target_fmax_mhz

  2. Updates tests so that tests use at least one removed spelling of each attributes and now shows a warning with unknown attribute spelling.

Signed-off-by: Soumi Manna [email protected]

@smanna12 smanna12 force-pushed the RemoveDeprecatedAttrSpelling branch from da4e140 to 0e98f87 Compare September 10, 2021 17:24
@smanna12 smanna12 marked this pull request as ready for review September 10, 2021 17:28
premanandrao
premanandrao reviewed Sep 10, 2021
View reviewed changes
Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

Looks good to me. A couple of questions:

  • are there any plans to change the identifier names? e.g. SYCLIntelFPGAIVDep?
  • there is one test change about the "implicit memory" tag that wasn't clear to me. I asked that inline too.

@smanna12
Copy link
Contributor Author

are there any plans to change the identifier names? e.g. SYCLIntelFPGAIVDep?

I am not aware of any discussion about this.
I am in favor of changing the identifier name like SYCLIntelIVDep and others because we do not use FPGA spelling anymore. I will leave this on the reviewers @premanandrao , @elizabethandrews, and @AaronBallman. If we agree to do this, i will make all identifier name changes in a separate PR because several tests and coding part needs to be updated as well.

@AaronBallman
Copy link
Contributor

are there any plans to change the identifier names? e.g. SYCLIntelFPGAIVDep?

I am not aware of any discussion about this.
I am in favor of changing the identifier name like SYCLIntelIVDep and others because we do not use FPGA spelling anymore. I will leave this on the reviewers @premanandrao , @elizabethandrews, and @AaronBallman. If we agree to do this, i will make all identifier name changes in a separate PR because several tests and coding part needs to be updated as well.

I'd be in support of doing a renaming in a separate patch, but it's low priority if it turns out to be a slog for some reason.

@premanandrao
Copy link
Contributor

but it's low priority if it turns out to be a slog for some reason.

Agreed.

@smanna12
Address review comments
087a2c5 
Signed-off-by: Soumi Manna <[email protected]>
@smanna12
remove extra change
f381fe2 
Signed-off-by: Soumi Manna <[email protected]>
AaronBallman
AaronBallman approved these changes Sep 14, 2021
View reviewed changes
Copy link
Contributor

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM

premanandrao
premanandrao approved these changes Sep 14, 2021
View reviewed changes
Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

LGTM

@smanna12
Copy link
Contributor Author

Thank you @premanandrao and @AaronBallman for the reviews.

@bader bader merged commit c8c5163 into intel:sycl Sep 15, 2021
@smanna12
Copy link
Contributor Author

smanna12 commented Sep 15, 2021 •
edited
Loading

Thanks @bader.

We have release notes - sycl/ReleaseNotes.md . They will be updated when we are close to release.

FYI, @romanovvlad, Please update release notes. Thank you.

alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Sep 18, 2021
@alexbatashev
Merge remote-tracking branch 'upstream/sycl' into xpti_tests_enable
bc2716c 
* upstream/sycl: (36 commits)
  [SYCL] Add SYCL2020 target::device enumeration value (intel#4587)
  [SYCL][Doc] Update ITT instrumentation docs (intel#4503)
  [SYCL][L0] Make all L0 events have device-visibility (intel#4534)
  [SYCL] Updated Level-Zero backend spec according to SYCL 2020 standard (intel#4560)
  [SYCL] Add error_code support for SYCL 1.2.1 exception classes (intel#4574)
  [SYCL][CI] Provide --ci-defaults option for config script (intel#4583)
  [CI] Switch GitHub Actions to Ubuntu 20.04 (intel#4582)
  [SYCL][CUDA] Fix context clearing in PiCuda tests (intel#4483)
  [SYCL] Hide SYCL service kernels (intel#4519)
  [SYCL][L0] Fix mismatched ZE call count (intel#4559)
  [SYCL] Remove function pointers extension (intel#4459)
  [GitHub Actions] Uplift clang version in post-commit validation (intel#4581)
  [SYCL] Ignore usm prefetch dummy flag (intel#4568)
  [SYCL][Group algorithms] Add group sorting algorithms implementation (intel#4439)
  [SYCL] Resolve name clash with a user defined symbol (intel#4570)
  [clang-offload-wrapper] Do not create .tgtimg section with -emit-reg-funcs=0 (intel#4577)
  [SYCL][FPGA] Remove deprecated attribute functionality (intel#4532)
  [SYCL] Remove _class aliases (intel#4465)
  [SYCL][CUDA][HIP] Report every device in its own platform (intel#4571)
  [SYCL][L0] make_device shouldn't need platform as an input (intel#4561)
  ...
@smanna12 smanna12 mentioned this pull request Dec 8, 2021
Closed
smanna12 added a commit to smanna12/llvm that referenced this pull request Nov 30, 2022
@smanna12
[SYCL][FPGA][NFC] Change the identifier names
afd8a8d 
This patch changes the identifier names because
we do not use FPGA spelling anymore (Ref:
intel#4532)

Fixes intel#5105
@smanna12 smanna12 mentioned this pull request Nov 30, 2022
Merged
pvchupin pushed a commit that referenced this pull request Dec 5, 2022
@smanna12
[SYCL][FPGA][NFC] Change the identifier names (#7596)
94894ee 
This patch changes the identifier names because
we do not use FPGA attribute spelling anymore (Ref:
#4532)

Fixes #5105

Signed-off-by: Soumi Manna
[[email protected]](mailto:[email protected])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AaronBallman AaronBallman AaronBallman approved these changes

@premanandrao premanandrao premanandrao approved these changes

@elizabethandrews elizabethandrews Awaiting requested review from elizabethandrews

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@smanna12 @AaronBallman @premanandrao @bader