Skip to content

[SYCL] Add support for new FPGA loop attribute nofusion #2715

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

Merged
merged 7 commits into from
Nov 5, 2020

Conversation

smanna12
Copy link
Contributor

@smanna12 smanna12 commented Nov 2, 2020

This patch adds support a new loop attribute for FPGA, intel::nofusion.
This attribute should be passed to the FPGA backend, and ignored by the emulator.
The attribute indicates that the annotated loop should not be fused with any adjacent loop.

Note: this does not include a corresponding [[intel::fusion]] attribute,
because a different mechanism (loop_fuse) will be built for FPGA.

Syntax:
[[intel::nofusion]]

The LLVM IR representation should be similar to the representation used for #pragma nofusion.
The llvm.loop metadata should specify llvm.loop.fusion.disable.

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

This patch adds support a new loop attribute for FPGA, intel::nofusion.
This attribute should be passed to the FPGA backend, and ignored by the emulator.
The attribute indicates that the annotated loop should not be fused with any adjacent loop.

Note: this does not include a corresponding [[intel::fusion]] attribute,
because a different mechanism (loop_fuse) will be built for FPGA.

Syntax:
[[intel::nofusion]]

The LLVM IR representation should be similar to the representation used for #pragma nofusion.
The llvm.loop metadata should specify llvm.loop.fusion.disable.

Signed-off-by: Soumi Manna <[email protected]>
Signed-off-by: Soumi Manna <[email protected]>
Signed-off-by: Soumi Manna <[email protected]>
Signed-off-by: Soumi Manna <[email protected]>
@smanna12 smanna12 marked this pull request as ready for review November 2, 2020 05:14
@@ -0,0 +1,64 @@
// RUN: %clang_cc1 -triple spir64-unknown-unknown-sycldevice -disable-llvm-passes -fsycl -fsycl-is-device -emit-llvm %s -o - | FileCheck %s
Copy link
Contributor

Choose a reason for hiding this comment

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

Do standard optimizations affect llvm.loop.fusion.disable metadata?
I'm asking because they are enabled by default, but the test disables them.
BTW, I'm not an expert in optimizations, but should we make sure that any early pre-SPIRV optimization didn't fuse any loops?

Copy link
Contributor Author

@smanna12 smanna12 Nov 2, 2020

Choose a reason for hiding this comment

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

@Fznamznon , Not sure i understand your question here. Without -disable-llvm-passes, i see this only for the same test In IR like this:

!4 = !{}
!5 = distinct !{!5, !6}
!6 = !{!"llvm.loop.fusion.disable"}

Copy link
Contributor

Choose a reason for hiding this comment

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

By default code which is produced by FE is optimized before translation to SPIR-V, then this SPIR-V is passed to underlying backend compiler. I guess this attribute is intended to give some information to the underlying backend compiler so it won't perform fusion on marked loops. I haven't seen the original request for this attribute, but it seems this attribute won't make any sense if loops somehow fused before translation to SPIR-V. What I'm trying to say is that we probably need to double check what we should do if there some standard optimization which is invoked before SPIR-V translation and can fuse some loop ignoring llvm.loop.fusion.disable metadata added by this patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I talked to Mariya offline. This concerns are resolved now.

Signed-off-by: Soumi Manna <[email protected]>
@smanna12 smanna12 requested a review from Fznamznon November 2, 2020 15:20
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

smanna12 commented Nov 3, 2020

Thank you everyone for the reviews.

@romanovvlad romanovvlad merged commit 68ab67a into intel:sycl Nov 5, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Nov 6, 2020
* upstream/sycl:
  [SYCL] Move tests with dependencies to on-device directory (intel#2732)
  [SYCL][Test] Remove leftovers for FPGA archives (intel#2735)
  [SYCL][NFC] Extend ABI tests to cover device code (intel#2725)
  [SYCL] Fix link to ESIMD tests (intel#2736)
  Added the SYCL_INTEL_mem_channel_property extension specification (intel#2688)
  [SYCL] Add support for new FPGA loop attribute nofusion (intel#2715)
  [SYCL] Remove host-task-dependency test added to llvm-test-suite (intel#2720)
  [SYCL] Remove warning about SYCL_EXTERNAL with pointers (intel#2722)
  [SYCL] dot_product support. (intel#2609)
  [SYCL][PI][L0] Fix a problem with kernels and programs destruction while they can be used (intel#2710)
  [SYCL] Fix the check for read-only host pointer during memobj creation (intel#2697)
smanna12 added a commit to smanna12/llvm that referenced this pull request Nov 12, 2020
This is a followup in intel#2715 (comment). This patch improves the
documentation by adding code examples for all FPGA loop attributes that
we did not have before.

Signed-off-by: Soumi Manna <[email protected]>
bader pushed a commit that referenced this pull request Nov 16, 2020
This is a followup in #2715 (comment). This patch improves the
documentation by adding code examples for all FPGA loop attributes that
we did not have before.

Signed-off-by: Soumi Manna <[email protected]>
jsji pushed a commit that referenced this pull request Sep 21, 2024
also fix formatting issue from a recent PR.

Signed-off-by: Sidorov, Dmitry <[email protected]>

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@9b0f29c488ada22
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