Skip to content
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

[SYCL] Enable mapping of group load/store functions to SPIRV built-ins for local address space #16653

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

againull
Copy link
Contributor

@againull againull commented Jan 16, 2025

Extension: https://registry.khronos.org/OpenCL/extensions/intel/cl_intel_subgroup_local_block_io.html

Currently these built-ins for local address space are not supported by cpu/fpga backends, so introduce undocumented native_local_block_io property which allows to enable mapping to those built-ins. If this property is not provided then implementation falls back to naive approach.

@againull againull requested a review from a team as a code owner January 16, 2025 00:20
@againull againull changed the title [SYCL] Extend OneAPI group load/store extension to work with local dress space [SYCL] Extend OneAPI group load/store extension to work with local address space Jan 16, 2025
@againull againull marked this pull request as draft January 16, 2025 16:58
…dress space

Currently intrinsics for local address space are not supported by
cpu/fpga backends, so introduce undocumented  native_local_block_io
property which allows to enable mapping to those intrinsics.
@againull againull force-pushed the group_load_store_local branch from 36e16ba to 622b2c7 Compare January 21, 2025 23:42
@againull againull marked this pull request as ready for review January 22, 2025 06:55
@againull againull changed the title [SYCL] Extend OneAPI group load/store extension to work with local address space [SYCL] Enable mapping of group load/store functions to SPIRV built-ins for local address space Jan 22, 2025
Copy link
Contributor

@cperkinsintel cperkinsintel left a comment

Choose a reason for hiding this comment

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

LGTM
we had that recent reversal and fix of striped vs. block which @aelovikov-intel had to deal with. I don't see any problem here, but maybe he should double check

@againull
Copy link
Contributor Author

LGTM we had that recent reversal and fix of striped vs. block which @aelovikov-intel had to deal with. I don't see any problem here, but maybe he should double check

Thank you! No, problems are not expected because PR relies on existing logic regarding this (i.e. striped vs block logic is the same for global and local ptr cases)

@againull againull merged commit 5edcf74 into intel:sycl Jan 24, 2025
17 checks passed
// CHECK-GLOBAL-NEXT: [[TMP0:%.*]] = load i64, ptr [[V:%.*]], align 8, !tbaa [[TBAA15]]
// CHECK-GLOBAL-NEXT: store i64 [[TMP0]], ptr [[AGG_TMP1]], align 8, !tbaa [[TBAA15]]
// CHECK-GLOBAL-NEXT: call void @llvm.memcpy.p0.p4.i64(ptr align 8 [[AGG_TMP2]], ptr addrspace(4) align 8 [[ITER:%.*]], i64 80, i1 false), !tbaa.struct [[TBAA_STRUCT92:![0-9]+]]
// CHECK-GLOBAL-NEXT: tail call spir_func void @_ZN4sycl3_V13ext6oneapi12experimental11group_storeINS0_9sub_groupEiLm2ENS0_6detail17accessor_iteratorIiLi1EEENS3_10propertiesINS3_6detail20properties_type_listIJNS3_14property_valueINS3_18data_placement_keyEJSt17integral_constantIiLi1EEEEENSC_INS3_21contiguous_memory_keyEJEEENSC_INS3_14full_group_keyEJEEENSC_INSA_25native_local_block_io_keyEJEEEEEEEEEENSt9enable_ifIXaaaasr6detailE18verify_store_typesIT0_T2_Esr6detailE18is_generic_group_vIT_E18is_property_list_vIT3_EEvE4typeESS_NS0_4spanISQ_XT1_EEESR_ST_(ptr noundef nonnull byval(%"struct.sycl::_V1::sub_group") align 1 [[AGG_TMP]], ptr noundef nonnull byval(%"class.sycl::_V1::span.22") align 8 [[AGG_TMP1]], ptr noundef nonnull byval(%"class.sycl::_V1::detail::accessor_iterator") align 8 [[AGG_TMP2]], ptr noundef nonnull byval(%"class.sycl::_V1::ext::oneapi::experimental::properties.28") align 1 [[AGG_TMP3]]) #[[ATTR7]]
Copy link
Contributor

@aelovikov-intel aelovikov-intel Feb 3, 2025

Choose a reason for hiding this comment

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

This looks wrong for the test_accessor_iter_force_optimized.

Update: my mistake, ignore.

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.

3 participants