Skip to content

[SYCL] Enable SPV_INTEL_fpga_invocation_pipelining_attributes extension #3864

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 4 commits into from
Jun 24, 2021

Conversation

MrSidims
Copy link
Contributor

@MrSidims MrSidims commented Jun 2, 2021

Signed-off-by: Dmitry Sidorov dmitry.sidorov@intel.com

@@ -8611,7 +8611,8 @@ void SPIRVTranslator::ConstructJob(Compilation &C, const JobAction &JA,
",+SPV_INTEL_arbitrary_precision_floating_point"
",+SPV_INTEL_variable_length_array,+SPV_INTEL_fp_fast_math_mode"
",+SPV_INTEL_fpga_cluster_attributes,+SPV_INTEL_loop_fuse"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is actually a gap in our design of how we enable SPIR-V extensions. I'm 100%, that Intel GPU doesn't support most of Intel FPGA extensions, but that issue is going to be addressed later.

Copy link
Contributor

Choose a reason for hiding this comment

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

If SPIR-V extension can only be triggered by corresponding DPC++ feature and we are clear in our documentation/intent that it is not supported on GPU, it should be fine

@MrSidims MrSidims requested a review from mlychkov June 2, 2021 09:59
AGindinson
AGindinson previously approved these changes Jun 2, 2021
@AGindinson AGindinson changed the title [SYCL] Enable SPV_INTEL_invocation_pipelining_attributes extension [SYCL] Enable SPV_INTEL_fpga_invocation_pipelining_attributes extension Jun 2, 2021
AGindinson
AGindinson previously approved these changes Jun 2, 2021
@MrSidims
Copy link
Contributor Author

MrSidims commented Jun 2, 2021

From the test failure:

Unknown extension 'SPV_INTEL_fpga_invocation_pipelining_attributes'

Indeed we need a pull-down first.

@bader bader marked this pull request as draft June 2, 2021 15:26
@bader
Copy link
Contributor

bader commented Jun 2, 2021

I converted this pull request to a draft to avoid unintentional merge.

@keryell
Copy link
Contributor

keryell commented Jun 3, 2021

I am curious about what it means for an FPGA attribute not to be supported on GPU or CPU or...
What happens if they are just ignored, like any other attribute?
At the end it would be nice to have a SYCL program just to work on any target and have functionality portability.

@AGindinson
Copy link
Contributor

I am curious about what it means for an FPGA attribute not to be supported on GPU or CPU or...
What happens if they are just ignored, like any other attribute?
At the end it would be nice to have a SYCL program just to work on any target and have functionality portability.

@keryell, if I understand correctly, the reference here is made to this comment thread about the fact that we enable a whole set of SPIR-V extensions for any target regardless of whether these extensions are supported by the underlying device runtimes. In this light, "ignoring" unknown SPIR-V capabilities is simply not how the device runtimes are currently taught to operate - and changing their behavior is not something we could do on the SYCL level. If LLVM BC and not SPIR-V was used as the interface between the FE and device backends, metadata-based attributes (i.e. most of the attributes) could truly be ignored by "irrelevant" runtimes. But the latter is an unrealistic architectural change with too many drawbacks.

We could, however, search for ways to disable FPGA-specific extensions for non-FPGA targets within the compiler driver, thus providing source portability of the functionality (if not runtime portability). At least this is how I've parsed @MrSidims's comment.

Also, it is often the case that users employing target-specific attributes like "FPGA pipelining" do not require their sources to be instantly portable from the chosen target and/or rely on the preprocessor to define different code paths for, say, FPGA and CPU. Overall, the concerns about some compiled device objects with target-tuned code not being executable on other targets don't seem to be severe for DPC++ users - unless, of course, I'm missing some data that shows otherwise.

@keryell
Copy link
Contributor

keryell commented Jun 3, 2021

At least just ignoring the FPGA features either in Clang or in the SYCL runtime is an easy.
I am claiming everywhere that a super cool feature of SYCL is that it allows to emulate FPGA code on GPU in a super-fast way compared to CPU or RTL simulation, so I would be sad this is not possible! :-)
What happens in the GPU SPIR-V consumer if the SPIR-V has FPGA extensions with no semantic changed could be allowed too.
But probably filtering them out earlier is clearer and some conditional warnings can be optionally emitted.

@MrSidims
Copy link
Contributor Author

MrSidims commented Jun 4, 2021

I am claiming everywhere that a super cool feature of SYCL is that it allows to emulate FPGA code on GPU in a super-fast way compared to CPU or RTL simulation, so I would be sad this is not possible! :-)

(if only we could emulate DPCPP pipes on GPU (well, we actually could, but it's not working right now) :) )
We still can do that, most of these extensions are added to translate a certain LLVM metadata to SPIR-V. And whilst metadata can be easily ignored and removed from the IR module it's not always the case for SPIR-V: we indeed can safely remove decorations, but not instructions. So it would be a bit hard to support FPGA SPIR-V extensions in non-FPGA SPIR-V-centric devices' backends, and also not every FPGA attribute can be used to optimize IR for non-FPGA targets. The fact that if an extension is disabled in the driver doesn't mean, that the translator will emit an error - instead it will change translation flow and don't generate IR patterns appropriate for the extension.

There are also extensions (for now it's the only SPV_INTEL_usm_storage_classes) that can't be just ignored as it adds translation of not-common OpenCL address spaces. And it seems to be reasonable to deal with it in the 'middle-end' instead of different devices' backends.

MrSidims added 4 commits June 23, 2021 16:15
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
@MrSidims MrSidims force-pushed the private/MrSidims/invocation_attr branch from 7faef23 to a5d901e Compare June 23, 2021 13:15
@MrSidims MrSidims marked this pull request as ready for review June 23, 2021 13:52
@MrSidims MrSidims requested review from AGindinson and mlychkov June 23, 2021 13:52
@bader bader merged commit 5dcf9a4 into intel:sycl Jun 24, 2021
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Jun 24, 2021
* upstream/sycl: (489 commits)
  [SYCL][NFC] Lower overhead of making plugin calls (intel#3982)
  [SYCL][NFC] Use default macro initialization where applicable (intel#3979)
  [SYCL] Enable SPV_INTEL_fpga_invocation_pipelining_attributes extension (intel#3864)
  [SYCL] Disable reassociate pass to reduce register pressure (intel#3615)
  [Driver][SYCL][FPGA] Restrict -O0 for FPGA with hardware (intel#3966)
  [SYCL][NFC] Fix warnings coming out of SYCL headers. (intel#3978)
  [SYCL] Fix bugs with recursion in SYCL kernel (intel#3958)
  [SYCL][LevelZero] Add support to detect host->device and device->host transfers for USM (intel#3975)
  [SYCL] Enable native FP atomics by default (intel#3869)
  [sycl-post-link] Avoid copying from nullptr (intel#3963)
  [SYCL-PTX] Add warp-reduce path in sub-group reduce (intel#3949)
  [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#3946)
  Fix handling of complex constant expressions
  Handle OpSpecConstantOp with CompositeExtract and CompositeInsert
  Handle OpSpecConstantOp with VectorShuffle
  [FuncSpec] Don't specialise functions with NoDuplicate instructions.
  [mlir][linalg] Support low padding in subtensor(pad_tensor) lowering
  [gn build] Port 208332d
  [AMDGPU] Add Optimize VGPR LiveRange Pass.
  [mlir][Linalg] NFC - Drop unused variable definition.
  ...
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