From 49e8c71b50a8c18bfe4c0f4b3523e9bc4fb2dc6b Mon Sep 17 00:00:00 2001 From: Nicolas Miller Date: Mon, 25 Apr 2022 15:28:40 +0100 Subject: [PATCH 1/7] [SYCL] Fix alignment of emulated specialization constants --- llvm/tools/sycl-post-link/SpecConstants.cpp | 41 +++++++++++++++++---- sycl/source/detail/device_image_impl.hpp | 36 +++++++++++++----- 2 files changed, 60 insertions(+), 17 deletions(-) diff --git a/llvm/tools/sycl-post-link/SpecConstants.cpp b/llvm/tools/sycl-post-link/SpecConstants.cpp index 30b2ff9a8e533..4ae7359c23ae7 100644 --- a/llvm/tools/sycl-post-link/SpecConstants.cpp +++ b/llvm/tools/sycl-post-link/SpecConstants.cpp @@ -425,7 +425,8 @@ void collectCompositeElementsDefaultValuesRecursive( MDNode *generateSpecConstantMetadata(const Module &M, StringRef SymbolicID, Type *SCTy, ArrayRef IDs, - bool IsNativeSpecConstant) { + bool IsNativeSpecConstant, + unsigned Padding = 0) { SmallVector MDOps; LLVMContext &Ctx = M.getContext(); auto *Int32Ty = Type::getInt32Ty(Ctx); @@ -457,9 +458,10 @@ MDNode *generateSpecConstantMetadata(const Module &M, StringRef SymbolicID, "There must be a single ID for emulated spec constant"); MDOps.push_back(ConstantAsMetadata::get( Constant::getIntegerValue(Int32Ty, APInt(32, IDs[0].ID)))); - // Second element is always zero here + // Second element is padding necessary to ensure alignment of spec constant + // in the buffer MDOps.push_back(ConstantAsMetadata::get( - Constant::getIntegerValue(Int32Ty, APInt(32, 0)))); + Constant::getIntegerValue(Int32Ty, APInt(32, Padding)))); unsigned Size = M.getDataLayout().getTypeStoreSize(SCTy); @@ -724,6 +726,7 @@ PreservedAnalyses SpecConstantsPass::run(Module &M, } bool IsNewSpecConstant = false; + unsigned Padding = 0; if (SetValAtRT) { // 2. Spec constant value will be set at run time - then add the literal // to a "spec const string literal ID" -> "vector of integer IDs" map, @@ -770,14 +773,27 @@ PreservedAnalyses SpecConstantsPass::run(Module &M, auto Ins = OffsetMap.insert(std::make_pair(SymID, NextOffset)); IsNewSpecConstant = Ins.second; unsigned CurrentOffset = Ins.first->second; - if (IsNewSpecConstant) { - unsigned Size = M.getDataLayout().getTypeStoreSize(SCTy); + unsigned Size = M.getDataLayout().getTypeStoreSize(SCTy); + unsigned Align = M.getDataLayout().getABITypeAlignment(SCTy); + + // Ensure the specialization constant is properly algined + if (CurrentOffset % Align != 0) { + Padding = Size - CurrentOffset % Align; + } + CurrentOffset += Padding; + + if (IsNewSpecConstant) { + // The CompositeOffset field is not used for specialization + // constant emulation so we can re-use it to communicate to the + // runtime the padding required for the specialization constant in + // the buffer SCMetadata[SymID] = generateSpecConstantMetadata( - M, SymID, SCTy, NextID, /* is native spec constant */ false); + M, SymID, SCTy, NextID, /* is native spec constant */ false, + Padding); ++NextID.ID; - NextOffset += Size; + NextOffset += Size + Padding; } Type *Int8Ty = Type::getInt8Ty(CI->getContext()); @@ -806,9 +822,18 @@ PreservedAnalyses SpecConstantsPass::run(Module &M, } } - if (IsNewSpecConstant && DefaultValue) + if (IsNewSpecConstant && DefaultValue) { + if (Padding != 0) { + // Initialize the padding with null data + LLVMContext &Ctx = DefaultValue->getContext(); + auto PadTy = ArrayType::get(Type::getInt8Ty(Ctx), Padding); + DefaultsMetadata.push_back(MDNode::get( + Ctx, + ConstantAsMetadata::get(llvm::Constant::getNullValue(PadTy)))); + } DefaultsMetadata.push_back( generateSpecConstDefaultValueMetadata(SymID, DefaultValue)); + } if (HasSretParameter) { // If __sycl_getCompositeSpecConstant returns through argument, then the diff --git a/sycl/source/detail/device_image_impl.hpp b/sycl/source/detail/device_image_impl.hpp index 4886c5c8ae25a..1ecb567cfda26 100644 --- a/sycl/source/detail/device_image_impl.hpp +++ b/sycl/source/detail/device_image_impl.hpp @@ -278,20 +278,38 @@ class device_image_impl { Descriptors.size()); unsigned LocalOffset = 0; while (It != End) { - // Make sure that alignment is correct in blob. - const unsigned OffsetFromLast = /*Offset*/ It[1] - LocalOffset; - BlobOffset += OffsetFromLast; - // Composites may have a special padding element at the end which - // should not have a descriptor. These padding elements all have max - // ID value. - if (It[0] != std::numeric_limits::max()) { + if (MBinImage->supportsSpecConstants()) { + // Make sure that alignment is correct in blob. + const unsigned OffsetFromLast = /*Offset*/ It[1] - LocalOffset; + BlobOffset += OffsetFromLast; + + // Composites may have a special padding element at the end which + // should not have a descriptor. These padding elements all have max + // ID value. + if (It[0] != std::numeric_limits::max()) { + // The map is not locked here because updateSpecConstSymMap() is + // only supposed to be called from c'tor. + MSpecConstSymMap[std::string{SCName}].push_back( + SpecConstDescT{/*ID*/ It[0], /*CompositeOffset*/ It[1], + /*Size*/ It[2], BlobOffset}); + } + + LocalOffset += OffsetFromLast + /*Size*/ It[2]; + } else { + // For emulated specialization constants the CompositeOffset is used + // to indicate necessary padding to ensure correct alignment. + BlobOffset += /* CompositeOffset */ It[1]; + // The map is not locked here because updateSpecConstSymMap() is // only supposed to be called from c'tor. + // Now that we've handled the padding we can set the + // CompositeOffset to 0, so that emulated and native spec constants + // can be handled the same going forward. MSpecConstSymMap[std::string{SCName}].push_back( - SpecConstDescT{/*ID*/ It[0], /*CompositeOffset*/ It[1], + SpecConstDescT{/*ID*/ It[0], /*CompositeOffset*/ 0, /*Size*/ It[2], BlobOffset}); } - LocalOffset += OffsetFromLast + /*Size*/ It[2]; + BlobOffset += /*Size*/ It[2]; It += NumElements; } From 0b7b634cf42d283b423b6ac42ad4a2e54b6767e3 Mon Sep 17 00:00:00 2001 From: Nicolas Miller Date: Tue, 10 May 2022 15:03:16 +0100 Subject: [PATCH 2/7] [SYCL] Update spec constant lit test --- .../spec-constants/SYCL-2020.ll | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll b/llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll index 15eaeb7545eca..e84b4710a3713 100644 --- a/llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll +++ b/llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll @@ -72,7 +72,7 @@ entry: ; CHECK-RT: call half @_Z20__spirv_SpecConstantiDh(i32 [[#SCID0:]], half 0xH4000) %call.i3 = tail call i32 @_Z37__sycl_getScalar2020SpecConstantValueIiET_PKcPvS3_(i8* getelementptr inbounds ([34 x i8], [34 x i8]* @__builtin_unique_stable_name._Z27get_specialization_constantIL_Z6id_intE17specialization_idIiEiET1_v, i64 0, i64 0), i8* bitcast (%class.specialization_id.0* @id_int to i8*), i8* null) -; CHECK-DEF: %[[GEP1:[0-9a-z]+]] = getelementptr i8, i8* null, i32 2 +; CHECK-DEF: %[[GEP1:[0-9a-z]+]] = getelementptr i8, i8* null, i32 4 ; CHECK-DEF: %[[BITCAST1:[0-9a-z]+]] = bitcast i8* %[[GEP1]] to i32* ; CHECK-DEF: %[[LOAD1:[0-9a-z]+]] = load i32, i32* %[[BITCAST1]], align 4 ; @@ -96,7 +96,7 @@ entry: %0 = bitcast %struct.ComposConst* %tmp to i8* call void @llvm.lifetime.start.p0i8(i64 24, i8* nonnull %0) #3 call void @_Z40__sycl_getComposite2020SpecConstantValueI11ComposConstET_PKcPvS4_(%struct.ComposConst* nonnull sret(%struct.ComposConst) align 8 %tmp, i8* getelementptr inbounds ([37 x i8], [37 x i8]* @__builtin_unique_stable_name._Z27get_specialization_constantIL_Z9id_composE17specialization_idI11ComposConstES1_ET1_v, i64 0, i64 0), i8* bitcast (%class.specialization_id.1* @id_compos to i8*), i8* null) -; CHECK-DEF: %[[GEP:[0-9a-z]+]] = getelementptr i8, i8* null, i32 6 +; CHECK-DEF: %[[GEP:[0-9a-z]+]] = getelementptr i8, i8* null, i32 8 ; CHECK-DEF: %[[BITCAST:[0-9a-z]+]] = bitcast i8* %[[GEP]] to %struct.ComposConst* ; CHECK-DEF: %[[C1:[0-9a-z]+]] = load %struct.ComposConst, %struct.ComposConst* %[[BITCAST]], align 8 ; @@ -113,7 +113,7 @@ entry: %1 = getelementptr inbounds %struct.ComposConst2, %struct.ComposConst2* %tmp1, i64 0, i32 0 call void @llvm.lifetime.start.p0i8(i64 24, i8* nonnull %1) #3 call void @_Z40__sycl_getComposite2020SpecConstantValueI12ComposConst2ET_PKcPvS4_(%struct.ComposConst2* nonnull sret(%struct.ComposConst2) align 8 %tmp1, i8* getelementptr inbounds ([39 x i8], [39 x i8]* @__builtin_unique_stable_name._Z27get_specialization_constantIL_Z10id_compos2E17specialization_idI12ComposConst2ES1_ET1_v, i64 0, i64 0), i8* getelementptr inbounds (%class.specialization_id.2, %class.specialization_id.2* @id_compos2, i64 0, i32 0, i32 0), i8* null) call void @llvm.lifetime.end.p0i8(i64 24, i8* nonnull %1) #3 -; CHECK-DEF: %[[GEP1:[0-9a-z]+]] = getelementptr i8, i8* null, i32 30 +; CHECK-DEF: %[[GEP1:[0-9a-z]+]] = getelementptr i8, i8* null, i32 32 ; CHECK-DEF: %[[BITCAST1:[0-9a-z]+]] = bitcast i8* %[[GEP1]] to %struct.ComposConst2* ; CHECK-DEF: %[[C2:[0-9a-z]+]] = load %struct.ComposConst2, %struct.ComposConst2* %[[BITCAST1]], align 8 ; @@ -129,7 +129,7 @@ entry: %2 = bitcast %struct.ComposConst* %tmp2 to i8* call void @llvm.lifetime.start.p0i8(i64 24, i8* nonnull %2) #3 call void @_Z40__sycl_getComposite2020SpecConstantValueI11ComposConstET_PKcPvS4_(%struct.ComposConst* nonnull sret(%struct.ComposConst) align 8 %tmp2, i8* getelementptr inbounds ([37 x i8], [37 x i8]* @__builtin_unique_stable_name._Z27get_specialization_constantIL_Z9id_composE17specialization_idI11ComposConstES1_ET1_v, i64 0, i64 0), i8* bitcast (%class.specialization_id.1* @id_compos to i8*), i8* null) -; CHECK-DEF: %[[GEP2:[0-9a-z]+]] = getelementptr i8, i8* null, i32 6 +; CHECK-DEF: %[[GEP2:[0-9a-z]+]] = getelementptr i8, i8* null, i32 8 ; CHECK-DEF: %[[BITCAST2:[0-9a-z]+]] = bitcast i8* %[[GEP2]] to %struct.ComposConst* ; CHECK-DEF: %[[C3:[0-9a-z]+]] = load %struct.ComposConst, %struct.ComposConst* %[[BITCAST2]], align 8 ; @@ -148,7 +148,7 @@ entry: define void @test_zeroinit() { %tmp = alloca %struct.ComposConst3, align 4 %1 = bitcast %struct.ComposConst3* %tmp to i8* -; CHECK-DEF: %[[GEP3:[0-9a-z]+]] = getelementptr i8, i8* null, i32 54 +; CHECK-DEF: %[[GEP3:[0-9a-z]+]] = getelementptr i8, i8* null, i32 56 ; CHECK-DEF: %[[BITCAST3:[0-9a-z]+]] = bitcast i8* %[[GEP3]] to %struct.ComposConst3* ; CHECK-DEF: %[[C3:[0-9a-z]+]] = load %struct.ComposConst3, %struct.ComposConst3* %[[BITCAST3]], align 4 ; @@ -169,7 +169,7 @@ define void @test3() { %tmp3 = alloca %struct.MArrayConst3, align 8 %tmp4 = alloca %struct.MArrayConst4, align 8 %1 = bitcast %struct.VectorConst* %tmp to i8* -; CHECK-DEF: %[[GEP1:[0-9a-z]+]] = getelementptr i8, i8* null, i32 70 +; CHECK-DEF: %[[GEP1:[0-9a-z]+]] = getelementptr i8, i8* null, i32 72 ; CHECK-DEF: %[[BITCAST1:[0-9a-z]+]] = bitcast i8* %[[GEP1]] to %struct.VectorConst* ; CHECK-DEF: %[[C1:[0-9a-z]+]] = load %struct.VectorConst, %struct.VectorConst* %[[BITCAST1]], align 8 ; @@ -179,7 +179,7 @@ define void @test3() { ; CHECK-RT: call %struct.VectorConst @_Z29__spirv_SpecConstantCompositeDv2_i_Rstruct.VectorConst(<2 x i32> %[[#CE1]]) call void @_Z40__sycl_getComposite2020SpecConstantValueI11VectorConstET_PKcPvS4_(%struct.VectorConst* nonnull sret(%struct.VectorConst) align 8 %tmp, i8* getelementptr inbounds ([38 x i8], [38 x i8]* @__builtin_unique_stable_name._Z27get_specialization_constantIL_Z10id_vectorE17specialization_idI11VectorConstES1_ET1_v, i64 0, i64 0), i8* bitcast (%class.specialization_id.3* @id_vector to i8*), i8* null) %2 = bitcast %struct.MArrayConst* %tmp1 to i8* -; CHECK-DEF: %[[GEP2:[0-9a-z]+]] = getelementptr i8, i8* null, i32 78 +; CHECK-DEF: %[[GEP2:[0-9a-z]+]] = getelementptr i8, i8* null, i32 80 ; CHECK-DEF: %[[BITCAST2:[0-9a-z]+]] = bitcast i8* %[[GEP2]] to %struct.MArrayConst* ; CHECK-DEF: %[[C2:[0-9a-z]+]] = load %struct.MArrayConst, %struct.MArrayConst* %[[BITCAST2]], align 4 ; @@ -234,11 +234,15 @@ attributes #3 = { nounwind } ; CHECK: !sycl.specialization-constants = !{![[#ID0:]], ![[#ID1:]], ![[#ID2:]], ![[#ID3:]], ![[#ID_COMPOS3:]], ![[#ID4:]], ![[#ID5:]] ; -; CHECK-DEF: !sycl.specialization-constants-default-values = !{![[#ID4:]], ![[#ID5:]], ![[#ID6:]], ![[#ID7:]], ![[#ID_COMPOS3_DEFAULT:]], ![[#ID8:]], ![[#ID9:]] +; CHECK-DEF: !sycl.specialization-constants-default-values = !{![[#ID4:]], ![[#ID5_PAD:]], ![[#ID5:]], ![[#ID6:]], ![[#ID7:]], ![[#ID_COMPOS3_DEFAULT:]], ![[#ID8:]], ![[#ID9:]] ; CHECK-RT: !sycl.specialization-constants-default-values ; ; CHECK: ![[#ID0]] = !{!"_ZTS14name_generatorIL_Z9id_halfEE", i32 0, i32 0, i32 2} -; CHECK: ![[#ID1]] = !{!"_ZTS14name_generatorIL_Z6id_intEE", i32 1, i32 0, i32 4} +; +; Emulated spec constant may use composite offset field for alignment padding +; +; CHECK-RT: ![[#ID1]] = !{!"_ZTS14name_generatorIL_Z6id_intEE", i32 1, i32 0, i32 4} +; CHECK-DEF: ![[#ID1]] = !{!"_ZTS14name_generatorIL_Z6id_intEE", i32 1, i32 2, i32 4} ; ; For composite types, the amount of metadata is a bit different between native and emulated spec constants ; @@ -256,6 +260,7 @@ attributes #3 = { nounwind } ; CHECK-RT-SAME: i32 [[#SCID9]], i32 16, i32 8} ; ; CHECK-DEF: ![[#ID4]] = !{half 0xH4000} +; CHECK-DEF: ![[#ID5_PAD]] = !{[2 x i8] zeroinitializer} ; CHECK-DEF: ![[#ID5]] = !{i32 42} ; CHECK-DEF: ![[#ID6]] = !{%struct.ComposConst { i32 1, double 2.000000e+00, %struct.myConst { i32 13, float 0x4020666660000000 } }} ; CHECK-DEF: ![[#ID7]] = !{%struct.ComposConst2 { i8 1, %struct.myConst { i32 52, float 0x40479999A0000000 }, double 2.000000e+00 }} From 83efbcf64deb6880c9480ebc3f2407b10c3ae928 Mon Sep 17 00:00:00 2001 From: Nicolas Miller Date: Thu, 12 May 2022 09:56:58 +0100 Subject: [PATCH 3/7] [SYCL] Fix typo --- llvm/tools/sycl-post-link/SpecConstants.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/tools/sycl-post-link/SpecConstants.cpp b/llvm/tools/sycl-post-link/SpecConstants.cpp index 4ae7359c23ae7..94f7456abe328 100644 --- a/llvm/tools/sycl-post-link/SpecConstants.cpp +++ b/llvm/tools/sycl-post-link/SpecConstants.cpp @@ -777,7 +777,7 @@ PreservedAnalyses SpecConstantsPass::run(Module &M, unsigned Size = M.getDataLayout().getTypeStoreSize(SCTy); unsigned Align = M.getDataLayout().getABITypeAlignment(SCTy); - // Ensure the specialization constant is properly algined + // Ensure the specialization constant is properly aligned if (CurrentOffset % Align != 0) { Padding = Size - CurrentOffset % Align; } From 5e3f2590f72b6b5a39b805c1913904e0369f72c4 Mon Sep 17 00:00:00 2001 From: Nicolas Miller Date: Thu, 12 May 2022 10:23:05 +0100 Subject: [PATCH 4/7] [SYCL] Refactor spec constant loop --- sycl/source/detail/device_image_impl.hpp | 37 ++++++++++++------------ 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/sycl/source/detail/device_image_impl.hpp b/sycl/source/detail/device_image_impl.hpp index 1ecb567cfda26..dfb874dc0fee1 100644 --- a/sycl/source/detail/device_image_impl.hpp +++ b/sycl/source/detail/device_image_impl.hpp @@ -278,39 +278,38 @@ class device_image_impl { Descriptors.size()); unsigned LocalOffset = 0; while (It != End) { + unsigned int ID = It[0]; + unsigned int CompositeOffset = It[1]; + unsigned int Size = It[2]; + if (MBinImage->supportsSpecConstants()) { // Make sure that alignment is correct in blob. - const unsigned OffsetFromLast = /*Offset*/ It[1] - LocalOffset; + const unsigned OffsetFromLast = CompositeOffset - LocalOffset; BlobOffset += OffsetFromLast; - // Composites may have a special padding element at the end which - // should not have a descriptor. These padding elements all have max - // ID value. - if (It[0] != std::numeric_limits::max()) { - // The map is not locked here because updateSpecConstSymMap() is - // only supposed to be called from c'tor. - MSpecConstSymMap[std::string{SCName}].push_back( - SpecConstDescT{/*ID*/ It[0], /*CompositeOffset*/ It[1], - /*Size*/ It[2], BlobOffset}); - } - - LocalOffset += OffsetFromLast + /*Size*/ It[2]; + LocalOffset += OffsetFromLast + Size; } else { // For emulated specialization constants the CompositeOffset is used // to indicate necessary padding to ensure correct alignment. - BlobOffset += /* CompositeOffset */ It[1]; + BlobOffset += CompositeOffset; - // The map is not locked here because updateSpecConstSymMap() is - // only supposed to be called from c'tor. // Now that we've handled the padding we can set the // CompositeOffset to 0, so that emulated and native spec constants // can be handled the same going forward. + CompositeOffset = 0; + } + + // Composites may have a special padding element at the end which + // should not have a descriptor. These padding elements all have max + // ID value. + if (ID != std::numeric_limits::max()) { + // The map is not locked here because updateSpecConstSymMap() is + // only supposed to be called from c'tor. MSpecConstSymMap[std::string{SCName}].push_back( - SpecConstDescT{/*ID*/ It[0], /*CompositeOffset*/ 0, - /*Size*/ It[2], BlobOffset}); + SpecConstDescT{ID, CompositeOffset, Size, BlobOffset}); } - BlobOffset += /*Size*/ It[2]; + BlobOffset += Size; It += NumElements; } } From 10ad3339a8b77a026a576cb2af17534f8e2ae54c Mon Sep 17 00:00:00 2001 From: Nicolas Miller Date: Mon, 16 May 2022 17:20:28 +0100 Subject: [PATCH 5/7] [SYCL] Handle alignment through padding element --- .../spec-constants/SYCL-2020.ll | 9 +- llvm/tools/sycl-post-link/SpecConstants.cpp | 89 ++++++++++++++----- sycl/source/detail/device_image_impl.hpp | 33 ++----- 3 files changed, 79 insertions(+), 52 deletions(-) diff --git a/llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll b/llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll index e84b4710a3713..f8d53f8ef966e 100644 --- a/llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll +++ b/llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll @@ -237,12 +237,11 @@ attributes #3 = { nounwind } ; CHECK-DEF: !sycl.specialization-constants-default-values = !{![[#ID4:]], ![[#ID5_PAD:]], ![[#ID5:]], ![[#ID6:]], ![[#ID7:]], ![[#ID_COMPOS3_DEFAULT:]], ![[#ID8:]], ![[#ID9:]] ; CHECK-RT: !sycl.specialization-constants-default-values ; -; CHECK: ![[#ID0]] = !{!"_ZTS14name_generatorIL_Z9id_halfEE", i32 0, i32 0, i32 2} +; Emulated spec constant may use an extra padding element to ensure alignment +; CHECK-RT: ![[#ID0]] = !{!"_ZTS14name_generatorIL_Z9id_halfEE", i32 0, i32 0, i32 2} +; CHECK-DEF: ![[#ID0]] = !{!"_ZTS14name_generatorIL_Z9id_halfEE", i32 0, i32 0, i32 2, i32 -1, i32 2, i32 2} ; -; Emulated spec constant may use composite offset field for alignment padding -; -; CHECK-RT: ![[#ID1]] = !{!"_ZTS14name_generatorIL_Z6id_intEE", i32 1, i32 0, i32 4} -; CHECK-DEF: ![[#ID1]] = !{!"_ZTS14name_generatorIL_Z6id_intEE", i32 1, i32 2, i32 4} +; CHECK: ![[#ID1]] = !{!"_ZTS14name_generatorIL_Z6id_intEE", i32 1, i32 0, i32 4} ; ; For composite types, the amount of metadata is a bit different between native and emulated spec constants ; diff --git a/llvm/tools/sycl-post-link/SpecConstants.cpp b/llvm/tools/sycl-post-link/SpecConstants.cpp index 94f7456abe328..cd6a5c937bf2e 100644 --- a/llvm/tools/sycl-post-link/SpecConstants.cpp +++ b/llvm/tools/sycl-post-link/SpecConstants.cpp @@ -425,8 +425,7 @@ void collectCompositeElementsDefaultValuesRecursive( MDNode *generateSpecConstantMetadata(const Module &M, StringRef SymbolicID, Type *SCTy, ArrayRef IDs, - bool IsNativeSpecConstant, - unsigned Padding = 0) { + bool IsNativeSpecConstant) { SmallVector MDOps; LLVMContext &Ctx = M.getContext(); auto *Int32Ty = Type::getInt32Ty(Ctx); @@ -458,10 +457,9 @@ MDNode *generateSpecConstantMetadata(const Module &M, StringRef SymbolicID, "There must be a single ID for emulated spec constant"); MDOps.push_back(ConstantAsMetadata::get( Constant::getIntegerValue(Int32Ty, APInt(32, IDs[0].ID)))); - // Second element is padding necessary to ensure alignment of spec constant - // in the buffer + // Second element is always zero here MDOps.push_back(ConstantAsMetadata::get( - Constant::getIntegerValue(Int32Ty, APInt(32, Padding)))); + Constant::getIntegerValue(Int32Ty, APInt(32, 0)))); unsigned Size = M.getDataLayout().getTypeStoreSize(SCTy); @@ -773,27 +771,74 @@ PreservedAnalyses SpecConstantsPass::run(Module &M, auto Ins = OffsetMap.insert(std::make_pair(SymID, NextOffset)); IsNewSpecConstant = Ins.second; unsigned CurrentOffset = Ins.first->second; - - unsigned Size = M.getDataLayout().getTypeStoreSize(SCTy); - unsigned Align = M.getDataLayout().getABITypeAlignment(SCTy); - - // Ensure the specialization constant is properly aligned - if (CurrentOffset % Align != 0) { - Padding = Size - CurrentOffset % Align; - } - CurrentOffset += Padding; - if (IsNewSpecConstant) { - // The CompositeOffset field is not used for specialization - // constant emulation so we can re-use it to communicate to the - // runtime the padding required for the specialization constant in - // the buffer + unsigned Size = M.getDataLayout().getTypeStoreSize(SCTy); + unsigned Align = M.getDataLayout().getABITypeAlignment(SCTy); + + // Ensure correct alignment + if (CurrentOffset % Align != 0) { + // Compute necessary padding to correctly align the constant. + Padding = Size - CurrentOffset % Align; + + // Update offsets. + NextOffset += Padding; + CurrentOffset += Padding; + OffsetMap[SymID] = NextOffset; + + // The spec constant map can't be empty as the first offset is 0 + // and so it can't be misaligned. + assert(!SCMetadata.empty() && + "Cannot add padding to first spec constant"); + + // To communicate the padding to the runtime, update the metadata + // node of the previous spec constant to append a padding node. It + // can't be added in front of the current spec constant, as doing + // so would require the spec constant node to have a non-zero + // CompositeOffset which breaks accessing it in the runtime. + auto Prev = SCMetadata.back(); + + // Emulated spec constants don't use composite so should + // always be formatted as (SymID, ID, Offset, Size), except when + // they include padding, but since padding is added at insertion + // of the next element, the last element of the map can never be + // padded. + assert(Prev.second->getNumOperands() == 4 && + "Incorrect emulated spec constant format"); + + LLVMContext &Ctx = M.getContext(); + auto *Int32Ty = Type::getInt32Ty(Ctx); + SmallVector MDOps; + + // Copy the existing metadata. + MDOps.push_back(Prev.second->getOperand(0)); + MDOps.push_back(Prev.second->getOperand(1)); + MDOps.push_back(Prev.second->getOperand(2)); + auto &SizeOp = Prev.second->getOperand(3); + MDOps.push_back(SizeOp); + + // Extract the size of the previous node to use as CompositeOffset + // for the padding node. + auto PrevSize = mdconst::extract(SizeOp)->getValue(); + + // The max value is a magic value used for padding that the + // runtime knows to skip. + MDOps.push_back(ConstantAsMetadata::get(Constant::getIntegerValue( + Int32Ty, APInt(32, std::numeric_limits::max())))); + MDOps.push_back(ConstantAsMetadata::get( + Constant::getIntegerValue(Int32Ty, PrevSize))); + MDOps.push_back(ConstantAsMetadata::get( + Constant::getIntegerValue(Int32Ty, APInt(32, Padding)))); + + // Replace previous metadata node with the node including the + // padding. + SCMetadata[Prev.first] = MDNode::get(Ctx, MDOps); + } + SCMetadata[SymID] = generateSpecConstantMetadata( - M, SymID, SCTy, NextID, /* is native spec constant */ false, - Padding); + M, SymID, SCTy, NextID, /* is native spec constant */ false); ++NextID.ID; - NextOffset += Size + Padding; + NextOffset += Size; } Type *Int8Ty = Type::getInt8Ty(CI->getContext()); diff --git a/sycl/source/detail/device_image_impl.hpp b/sycl/source/detail/device_image_impl.hpp index dfb874dc0fee1..4886c5c8ae25a 100644 --- a/sycl/source/detail/device_image_impl.hpp +++ b/sycl/source/detail/device_image_impl.hpp @@ -278,38 +278,21 @@ class device_image_impl { Descriptors.size()); unsigned LocalOffset = 0; while (It != End) { - unsigned int ID = It[0]; - unsigned int CompositeOffset = It[1]; - unsigned int Size = It[2]; - - if (MBinImage->supportsSpecConstants()) { - // Make sure that alignment is correct in blob. - const unsigned OffsetFromLast = CompositeOffset - LocalOffset; - BlobOffset += OffsetFromLast; - - LocalOffset += OffsetFromLast + Size; - } else { - // For emulated specialization constants the CompositeOffset is used - // to indicate necessary padding to ensure correct alignment. - BlobOffset += CompositeOffset; - - // Now that we've handled the padding we can set the - // CompositeOffset to 0, so that emulated and native spec constants - // can be handled the same going forward. - CompositeOffset = 0; - } - + // Make sure that alignment is correct in blob. + const unsigned OffsetFromLast = /*Offset*/ It[1] - LocalOffset; + BlobOffset += OffsetFromLast; // Composites may have a special padding element at the end which // should not have a descriptor. These padding elements all have max // ID value. - if (ID != std::numeric_limits::max()) { + if (It[0] != std::numeric_limits::max()) { // The map is not locked here because updateSpecConstSymMap() is // only supposed to be called from c'tor. MSpecConstSymMap[std::string{SCName}].push_back( - SpecConstDescT{ID, CompositeOffset, Size, BlobOffset}); + SpecConstDescT{/*ID*/ It[0], /*CompositeOffset*/ It[1], + /*Size*/ It[2], BlobOffset}); } - - BlobOffset += Size; + LocalOffset += OffsetFromLast + /*Size*/ It[2]; + BlobOffset += /*Size*/ It[2]; It += NumElements; } } From 4f16bc99b39d70606ac5b8a82728012e004cefc2 Mon Sep 17 00:00:00 2001 From: Nicolas Miller Date: Tue, 17 May 2022 17:37:25 +0100 Subject: [PATCH 6/7] [SYCL] Fix alignment calculation for spec constant --- llvm/tools/sycl-post-link/SpecConstants.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/llvm/tools/sycl-post-link/SpecConstants.cpp b/llvm/tools/sycl-post-link/SpecConstants.cpp index cd6a5c937bf2e..afad8df3b2398 100644 --- a/llvm/tools/sycl-post-link/SpecConstants.cpp +++ b/llvm/tools/sycl-post-link/SpecConstants.cpp @@ -778,13 +778,16 @@ PreservedAnalyses SpecConstantsPass::run(Module &M, // Ensure correct alignment if (CurrentOffset % Align != 0) { // Compute necessary padding to correctly align the constant. - Padding = Size - CurrentOffset % Align; + Padding = Align - CurrentOffset % Align; // Update offsets. NextOffset += Padding; CurrentOffset += Padding; OffsetMap[SymID] = NextOffset; + assert(CurrentOffset % Align == 0 && + "Alignment calculation error"); + // The spec constant map can't be empty as the first offset is 0 // and so it can't be misaligned. assert(!SCMetadata.empty() && From db2e52910b3b714bbd2e8b69f7ab2a1ccf46e615 Mon Sep 17 00:00:00 2001 From: Nicolas Miller Date: Tue, 17 May 2022 18:06:53 +0100 Subject: [PATCH 7/7] [SYCL] Fix bool spec constant lit test alignment --- llvm/test/tools/sycl-post-link/spec-constants/bool.ll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/test/tools/sycl-post-link/spec-constants/bool.ll b/llvm/test/tools/sycl-post-link/spec-constants/bool.ll index e02c5a4291d59..3ecaa866dc1c3 100644 --- a/llvm/test/tools/sycl-post-link/spec-constants/bool.ll +++ b/llvm/test/tools/sycl-post-link/spec-constants/bool.ll @@ -14,7 +14,7 @@ ; CHECK-LABEL: void @kernel_B ; CHECK-RT: call i8 @_Z20__spirv_SpecConstantia(i32 [[#]], i8 ; -; CHECK-DEF: %[[GEP:gep.*]] = getelementptr i8, i8 addrspace(4)* null, i32 1 +; CHECK-DEF: %[[GEP:gep.*]] = getelementptr i8, i8 addrspace(4)* null, i32 4 ; CHECK-DEF: %[[BC:bc.*]] = bitcast i8 addrspace(4)* %[[GEP]] to %struct.user_type addrspace(4)* ; CHECK-DEF: %[[LOAD:load.*]] = load %struct.user_type, %struct.user_type addrspace(4)* %[[BC]], align 4