Skip to content

[SYCL] Enable SPV_INTEL_fpga_invocation_pipelining_attributes extension - #3864

Merged
bader merged 4 commits into
intel:syclfrom
MrSidims:private/MrSidims/invocation_attr
Jun 24, 2021
Merged

[SYCL] Enable SPV_INTEL_fpga_invocation_pipelining_attributes extension#3864
bader merged 4 commits into
intel:syclfrom
MrSidims:private/MrSidims/invocation_attr

Conversation

@MrSidims

@MrSidims MrSidims commented Jun 2, 2021

Copy link
Copy Markdown
Contributor

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

Comment thread clang/lib/Driver/ToolChains/Clang.cpp Outdated

Copy link
Copy Markdown
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
Copy Markdown
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
Comment thread clang/test/Driver/sycl-spirv-ext.c Outdated
Comment thread clang/lib/Driver/ToolChains/Clang.cpp Outdated
Comment thread clang/test/Driver/sycl-spirv-ext.c Outdated
Comment thread clang/test/Driver/sycl-spirv-ext.c Outdated
Comment thread clang/test/Driver/sycl-spirv-ext.c Outdated
@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

MrSidims commented Jun 2, 2021

Copy link
Copy Markdown
Contributor Author

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

bader commented Jun 2, 2021

Copy link
Copy Markdown
Contributor

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

@keryell

keryell commented Jun 3, 2021

Copy link
Copy Markdown
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.

@AGindinson

Copy link
Copy Markdown
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

keryell commented Jun 3, 2021

Copy link
Copy Markdown
Contributor

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

MrSidims commented Jun 4, 2021

Copy link
Copy Markdown
Contributor Author

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.

Dmitry Sidorov 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