[SYCL-SPIRV] Add SPIR-V variants of TypeSampledImage as clang builtin type #1945

Naghasan · 2020-06-22T16:45:37Z

This patch adds SPIR-V sampled image types as derivative of the builtin OpenCL Image types.
For each OpenCL image type, clang defines a Sampled variant and
lowered as a "spirv.SampledImage." llvm opaque type.

The motivation for this patch is to allow a cleaner emulation of sampled image reads for the CUDA backend as it does not support independent image and sampler definition. This change allows to properly implement the __spirv_SampledImage builtin function by having it return a sampled image type (which can then be properly lowered according to its target) rather than void *.

Signed-off-by: Victor Lomuller victor@codeplay.com

…in clang. This patch adds SPIR-V sampled image types as derivative of the builtin OpenCL Image types. For each OpenCL image type, clang defines a Sampled<image type> variant and lowered as a "spirv.SampledImage.<image>" llvm opaque type. Signed-off-by: Victor Lomuller <victor@codeplay.com>

Fznamznon · 2020-06-23T10:46:55Z

@erichkeane, @bader , could you please help with review?

erichkeane

The code itself seems pretty mechanical and thus non controversial to me. That said, someone more familiar with what this is doing should judge/approve based on the solution chosen for the problem (that is, does adding these types fix the issue in the best way).

Naghasan · 2020-06-23T16:45:43Z

A bit more background for this patch: This patch makes the SPIR-V type OpTypeSampledImage (https://www.khronos.org/registry/spir-v/specs/unified1/SPIRV.html#OpTypeSampledImage) accessible in clang, even if the naming follows the OpenCL terminology (I think it makes a bit more sense from an upstreaming point of view).

This allow an accurate expression of OpSampledImage (https://www.khronos.org/registry/spir-v/specs/unified1/SPIRV.html#OpSampledImage) which in turns allow its implementation in a library. OpTypeSampledImage can be mapped into something meaningful for the target whereas void * can't.

Naghasan · 2020-06-25T10:29:31Z

PR #1978 is showing the application for this new type

bader · 2020-06-29T12:23:28Z

@intel/llvm-reviewers-runtime, @elizabethandrews, @Fznamznon, @premanandrao, ping.

clang/lib/Sema/SemaType.cpp

sycl/include/CL/__spirv/spirv_types.hpp

Signed-off-by: Victor Lomuller <victor@codeplay.com>

Fznamznon · 2020-07-03T15:09:54Z

clang/test/CodeGenOpenCL/sampled_image.cl

@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 %s -triple x86_64-unknown-linux-gnu -O0 -emit-llvm -o - | FileCheck %s


Is it intention to test it with x86 triple?

yes-ish, it follows the same patterns as for image.cl. This is just to make the output stable when it comes to running the tests.

s-kanaev

Runtime library headers look good to me.

premanandrao

There are multiple places where the IMAGE_TYPE macro is redefined. This would produce a build warning.

Other than that, looks good to me.

Naghasan · 2020-07-06T08:05:30Z

There are multiple places where the IMAGE_TYPE macro is redefined. This would produce a build warning.

OpenCLImageTypes.def undefines it, so no warning

Fznamznon · 2021-08-10T11:28:42Z

If I remember correctly, the original idea of using functions with __spirv prefixes and Spirv.* opaque types was inspired by SPIR-V friendly LLVM IR ( https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/docs/SPIRVRepresentationInLLVM.rst ) which allows to define functions/types in source code that will be correctly mapped to SPIR-V instructions/types by SPIR-V translator tool. SPIR-V friendly LLVM IR doc also defines how to attach additional image info to SampledImage type, see https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/docs/SPIRVRepresentationInLLVM.rst#optypesampledimage . I wonder what was the reason to use mix of OpenCL and SPIRV friendly types (I mean spirv.SampledImage.image*) instead of full SPIR-V friendly type (spirv.SampledImage._void_*)? Can we actually emit full SPIR-V friendly type?

These types are introduced in intel#1945

These types are introduced in #1945 This PR fix following build warnings: ``` [3204/3298] Building CXX object tools/lldb/source/Plugins/TypeSystem/Clang/CMakeFiles/lldbPluginTypeSystemClang.dir/TypeSystemClang.cpp.o lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp: In member function ‘virtual lldb::Encoding lldb_private::TypeSystemClang::GetEncoding(lldb::opaque_compiler_type_t, uint64_t&)’: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4829:12: warning: enumeration value ‘SampledOCLImage1dRO’ not handled in switch [-Wswitch] switch (llvm::cast<clang::BuiltinType>(qual_type)->getKind()) { ^ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4829:12: warning: enumeration value ‘SampledOCLImage1dArrayRO’ not handled in switch [-Wswitch] lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4829:12: warning: enumeration value ‘SampledOCLImage1dBufferRO’ not handled in switch [-Wswitch] lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4829:12: warning: enumeration value ‘SampledOCLImage2dRO’ not handled in switch [-Wswitch] lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4829:12: warning: enumeration value ‘SampledOCLImage2dArrayRO’ not handled in switch [-Wswitch] lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4829:12: warning: enumeration value ‘SampledOCLImage2dDepthRO’ not handled in switch [-Wswitch] lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4829:12: warning: enumeration value ‘SampledOCLImage2dArrayDepthRO’ not handled in switch [-Wswitch] lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4829:12: warning: enumeration value ‘SampledOCLImage2dMSAARO’ not handled in switch [-Wswitch] lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4829:12: warning: enumeration value ‘SampledOCLImage2dArrayMSAARO’ not handled in switch [-Wswitch] lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4829:12: warning: enumeration value ‘SampledOCLImage2dMSAADepthRO’ not handled in switch [-Wswitch] lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4829:12: warning: enumeration value ‘SampledOCLImage2dArrayMSAADepthRO’ not handled in switch [-Wswitch] lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4829:12: warning: enumeration value ‘SampledOCLImage3dRO’ not handled in switch [-Wswitch] ```

Naghasan requested review from elizabethandrews, Fznamznon, premanandrao and a team as code owners June 22, 2020 16:45

Naghasan requested a review from sergey-semenov June 22, 2020 16:45

Fznamznon requested review from bader and erichkeane June 23, 2020 10:47

erichkeane previously approved these changes Jun 23, 2020

View reviewed changes

This was referenced Jun 24, 2020

[SYCL][CUDA] Implemented sampled_image_cuda.cl #1972

Closed

[SYCL][CUDA][WIP] Lower OpenCL image type to CUDA image descriptors #1978

Merged

bader previously approved these changes Jun 27, 2020

View reviewed changes

bader reviewed Jun 29, 2020

View reviewed changes

clang/lib/Sema/SemaType.cpp Outdated Show resolved Hide resolved

s-kanaev reviewed Jun 29, 2020

View reviewed changes

sycl/include/CL/__spirv/spirv_types.hpp Show resolved Hide resolved

Fznamznon previously approved these changes Jun 29, 2020

View reviewed changes

[SYCL-PTX] Address review comments

96ab694

Signed-off-by: Victor Lomuller <victor@codeplay.com>

Naghasan dismissed stale reviews from Fznamznon, bader, and erichkeane via 96ab694 July 3, 2020 14:23

bader approved these changes Jul 3, 2020

View reviewed changes

Fznamznon reviewed Jul 3, 2020

View reviewed changes

bader added the cuda CUDA back-end label Jul 3, 2020

s-kanaev approved these changes Jul 3, 2020

View reviewed changes

bader requested a review from Fznamznon July 3, 2020 20:08

premanandrao reviewed Jul 4, 2020

View reviewed changes

bader requested a review from premanandrao July 6, 2020 09:10

bader merged commit 768f74f into intel:sycl Jul 6, 2020

bader mentioned this pull request Jan 20, 2021

[Upstream] Handle sample image types. #3065

Open

wenju-he added a commit to wenju-he/llvm that referenced this pull request Jan 11, 2023

[lldb] Fix build warnings of unhandled SPIRV builtin types

50c2856

These types are introduced in intel#1945

wenju-he mentioned this pull request Jan 11, 2023

[lldb] Fix build warnings of unhandled SPIRV builtin types #7978

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL-SPIRV] Add SPIR-V variants of TypeSampledImage as clang builtin type #1945

[SYCL-SPIRV] Add SPIR-V variants of TypeSampledImage as clang builtin type #1945

Naghasan commented Jun 22, 2020

Fznamznon commented Jun 23, 2020

erichkeane left a comment

Naghasan commented Jun 23, 2020

Naghasan commented Jun 25, 2020

bader commented Jun 29, 2020

Fznamznon Jul 3, 2020

Naghasan Jul 3, 2020

s-kanaev left a comment

premanandrao left a comment

Naghasan commented Jul 6, 2020

Fznamznon commented Aug 10, 2021 •

edited

Loading

		@@ -0,0 +1,12 @@
		// RUN: %clang_cc1 %s -triple x86_64-unknown-linux-gnu -O0 -emit-llvm -o - \| FileCheck %s

[SYCL-SPIRV] Add SPIR-V variants of TypeSampledImage as clang builtin type #1945

[SYCL-SPIRV] Add SPIR-V variants of TypeSampledImage as clang builtin type #1945

Conversation

Naghasan commented Jun 22, 2020

Fznamznon commented Jun 23, 2020

erichkeane left a comment

Choose a reason for hiding this comment

Naghasan commented Jun 23, 2020

Naghasan commented Jun 25, 2020

bader commented Jun 29, 2020

Fznamznon Jul 3, 2020

Choose a reason for hiding this comment

Naghasan Jul 3, 2020

Choose a reason for hiding this comment

s-kanaev left a comment

Choose a reason for hiding this comment

premanandrao left a comment

Choose a reason for hiding this comment

Naghasan commented Jul 6, 2020

Fznamznon commented Aug 10, 2021 • edited Loading

Fznamznon commented Aug 10, 2021 •

edited

Loading