[SYCL] Remove FPGA features from SYCL FE#21102
[SYCL] Remove FPGA features from SYCL FE#21102premanandrao wants to merge 2 commits intointel:syclfrom
Conversation
This removes the following attributes: [[intel::max_work_group_size]] [[intel::max_global_work_dim]] [[intel::no_global_work_offset]] [[intel::num_simd_work_items]] [[intel::doublepump]] [[intel::singlepump]] [[intel::fpga_memory]] [[intel::fpga_register]] [[intel::bankwidth]] [[intel::numbanks]] [[intel::private_copies]] [[intel::merge]] [[intel::max_replicates]] [[intel::simple_dual_port]] [[intel::bank_bits]] [[intel::force_pow2_depth]] [[intel::use_stall_enable_clusters]] [[intel::scheduler_target_fmax_mhz]] [[intel::initiation_interval]] [[intel::max_concurrency]] [[intel::loop_coalesce]] [[intel::disable_loop_pipelining]] [[intel::loop_count[_min|_max_avg]]] [[intel::max_interleaving]] [[intel::speculated_iterations]] [[intel::max_reinvocation_delay]] [[intel::enable_loop_pipelining]] [[intel::loop_fuse]] [[intel::loop_fuse_independent]] __attribute__((register_num(n))) __attribute__((pipe(mode))) __attribute__((io_pipe_id)))
cabf6db to
f98b027
Compare
Fznamznon
left a comment
There was a problem hiding this comment.
I don't think ESIMD attributes should be affected by this patch.
| let SimpleHandler = 1; | ||
| } | ||
|
|
||
| // Available in SYCL explicit SIMD extension. Binds a file scope private |
There was a problem hiding this comment.
That seems to be ESIMD attribute, i.e. specific for GPUs. Why are we removing this?
| let SupportsNonconformingLambdaSyntax = 1; | ||
| } | ||
|
|
||
| def SYCLIntelNumSimdWorkItems : InheritableAttr { |
There was a problem hiding this comment.
Same here, num_simd_work_items doesn't seem FPGA specific
| let SupportsNonconformingLambdaSyntax = 1; | ||
| } | ||
|
|
||
| def SYCLIntelMaxWorkGroupSize : InheritableAttr { |
There was a problem hiding this comment.
max_work_group_size doesn't seem specific for FPGA.
There was a problem hiding this comment.
This attribute was added in #883, where it is documented as FPGA-related. The only documentation I can find for this attribute is also FPGA-specific:
- https://www.intel.com/content/www/us/en/docs/oneapi-fpga-add-on/developer-guide/2024-2/specify-a-work-group-size.html
- https://www.intel.com/content/www/us/en/docs/oneapi-fpga-add-on/optimization-guide/2023-2/specify-a-work-group-size.html
When a kernel is decorated with this C++ attribute, the compiler adds the MaxWorkgroupSizeINTEL execution mode to the kernel's SPIR-V. @vmustya and @bashbaug, does IGC do anything useful when this execution mode is present on a kernel which runs on GPU?
There was a problem hiding this comment.
I'm not too familiar with the scalar IGC. @PawelJurek, could you please comment?
There was a problem hiding this comment.
I did a quick test and it doesn't look like IGC is (currently) rejecting SPIR-V modules that declare use of the SPV_INTEL_kernel_attributes extension and apply the MaxWorkgroupSizeINTEL execution mode, but it doesn't look like we are doing anything with it either, and I am able to enqueue kernels with a local work size that exceeds the specified values.
I also did a quick search through the IGC repo and didn't see any place where we are checking for this execution mode, either via the symbolic name MaxWorkgroupSizeINTEL or the ID value 5893.
For completeness, our CPU device also rejects modules that declare the KernelAttributesINTEL capability:
Compilation started
Unsupported SPIR-V module
SPIRV module requires unsupported capability 5892
Compilation failed
So in summary: Although the attribute could in theory be useful to non-FPGA devices, I don't think we're doing anything with it right now.
There was a problem hiding this comment.
Thanks, @bashbaug. This confirms my belief that we added this attribute for FPGA, and I think it makes sense to remove it. I do not think we should keep it around just because it might be useful someday. We can always add it back later if we find a need. In any case, we probably wouldn't use a C++ attribute even if we did find some other use for this. We'd instead add a compile-time kernel property.
BTW, we also have a SYCL compile-time property max_work_group_size which sets the same SPIR-V execution mode. I'll file a tracker to deprecate/remove that too. (This will affect the SYCL runtime headers, not the CFE.)
AaronBallman
left a comment
There was a problem hiding this comment.
Is there an external list of attributes we can compare this against (like in a Jira where someone identified the attributes)? The patch summary has a list, but it's hard to know whether that list was generated from what you removed or whether you removed attributes starting from that list. (Asking because there are questions about some of the attributes being removed and whether it's appropriate.)
I believe @premanandrao was following the list I created in the Jira CMPLRLLVM-68534. Note that I did say this when I created the list:
|
sarnex
left a comment
There was a problem hiding this comment.
+1 mariya i dont think we should be removing esimd attributes
The list came from @gmlueck, but I tried to lookup Jira requests pertaining to each attribute for context. But based on internal discussions, I will split this PR into multiple ones - one for each attribute or a small set of related attributes. That should make reviewing/reverting easier. Thanks for all the review comments so far. I will apply some changes now, but then close the PR. |
This removes the following attributes:
[[intel::max_work_group_size]]
[[intel::max_global_work_dim]]
[[intel::no_global_work_offset]]
[[intel::num_simd_work_items]]
[[intel::doublepump]]
[[intel::singlepump]]
[[intel::fpga_memory]]
[[intel::fpga_register]]
[[intel::bankwidth]]
[[intel::numbanks]]
[[intel::private_copies]]
[[intel::merge]]
[[intel::max_replicates]]
[[intel::simple_dual_port]]
[[intel::bank_bits]]
[[intel::force_pow2_depth]]
[[intel::use_stall_enable_clusters]]
[[intel::scheduler_target_fmax_mhz]]
[[intel::initiation_interval]]
[[intel::max_concurrency]]
[[intel::loop_coalesce]]
[[intel::disable_loop_pipelining]]
[[intel::loop_count[_min|_max_avg]]]
[[intel::max_interleaving]]
[[intel::speculated_iterations]]
[[intel::max_reinvocation_delay]]
[[intel::enable_loop_pipelining]]
[[intel::loop_fuse]]
[[intel::loop_fuse_independent]]
attribute((register_num(n)))
attribute((pipe(mode)))
attribute((io_pipe_id)))