From 759235a950e810446869dd1e136a5faa80e45120 Mon Sep 17 00:00:00 2001 From: Fraser Cormack Date: Mon, 7 Aug 2023 17:13:58 +0100 Subject: [PATCH 1/2] [host] Fix calculated size of sampler args --- modules/mux/targets/host/source/command_buffer.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/mux/targets/host/source/command_buffer.cpp b/modules/mux/targets/host/source/command_buffer.cpp index f242ce7e1..07cad8ca4 100644 --- a/modules/mux/targets/host/source/command_buffer.cpp +++ b/modules/mux/targets/host/source/command_buffer.cpp @@ -41,6 +41,8 @@ size_t calcPackedArgsAllocSize( size_t size = 0; switch (descriptor.type) { case mux_descriptor_info_type_sampler: + size = sizeof(uint32_t); + break; case mux_descriptor_info_type_buffer: case mux_descriptor_info_type_null_buffer: case mux_descriptor_info_type_image: From 1b2971d75d7d27c653aedd4fa397e9e5f3bfc8e4 Mon Sep 17 00:00:00 2001 From: Fraser Cormack Date: Mon, 7 Aug 2023 17:13:38 +0100 Subject: [PATCH 2/2] [compiler] Fix missing parameter attrs on sampler wrappers This fixes a bug introduced recently, where the wrappers we create (before LLVM 17) were losing the parameter attributes from the original kernel. This could lead to incorrect codegen in certain cases, most notably when we lose `byval` attributes. --- .../source/image_argument_substitution_pass.cpp | 17 +++++++++++++++++ .../compiler/test/lit/passes/image-arg-subst.ll | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/modules/compiler/source/base/source/image_argument_substitution_pass.cpp b/modules/compiler/source/base/source/image_argument_substitution_pass.cpp index b64b87269..2b28cd551 100644 --- a/modules/compiler/source/base/source/image_argument_substitution_pass.cpp +++ b/modules/compiler/source/base/source/image_argument_substitution_pass.cpp @@ -130,21 +130,38 @@ PreservedAnalyses compiler::ImageArgumentSubstitutionPass::run( IRBuilder<> B(BasicBlock::Create(Ctx, "entry", WrapperKernel)); SmallVector Args; + // Our wrapper hasn't been created with any parameter attributes, as the + // parameter types have changed. We must copy across all attributes from + // the non-sampler arguments to maintain program semantics. + AttributeList KernelAttrs = KernelF->getAttributes(); + SmallVector WrapperParamAttrs(KernelF->arg_size()); + for (auto [OldArg, NewArg] : zip(KernelF->args(), WrapperKernel->args())) { // Copy parameter names across + unsigned ArgIdx = Args.size(); NewArg.setName(OldArg.getName()); if (OldArg.getType() == NewArg.getType()) { Args.push_back(&NewArg); + WrapperParamAttrs[ArgIdx] = KernelAttrs.getParamAttrs(ArgIdx); } else { // Must be a sampler: simply cast the i32 to a pointer. This mirrors // what we'll do down the line when changing the pointer back to an // i32 via the opposite operation. Args.push_back(B.CreateIntToPtr(&NewArg, OldArg.getType(), NewArg.getName() + ".ptrcast")); + // Don't copy across any parameter attributes here, as this was a + // pointer and is now an i32. + WrapperParamAttrs[ArgIdx] = AttributeSet(); } } + // Set our parameter attributes, but maintain the function/return + // attributes the wrapper has already received. + WrapperKernel->setAttributes(AttributeList::get( + KernelF->getContext(), WrapperKernel->getAttributes().getFnAttrs(), + WrapperKernel->getAttributes().getRetAttrs(), WrapperParamAttrs)); + auto *const CI = B.CreateCall(KernelF, Args); CI->setCallingConv(KernelF->getCallingConv()); diff --git a/modules/compiler/test/lit/passes/image-arg-subst.ll b/modules/compiler/test/lit/passes/image-arg-subst.ll index a6b9dd22c..99b0600e8 100644 --- a/modules/compiler/test/lit/passes/image-arg-subst.ll +++ b/modules/compiler/test/lit/passes/image-arg-subst.ll @@ -56,7 +56,7 @@ entry: } ; We've updated the old kernel to pass samplers as i32 -; CHECK: define spir_kernel void @image_sampler(ptr addrspace(1) %out, ptr addrspace(1) %img, i32 %sampler1, i32 %sampler2) [[NEW_ATTRS:#[0-9]+]] { +; CHECK: define spir_kernel void @image_sampler(ptr addrspace(1) nocapture writeonly align 4 %out, ptr addrspace(1) %img, i32 %sampler1, i32 %sampler2) [[NEW_ATTRS:#[0-9]+]] { ; CHECK: %sampler1.ptrcast = inttoptr i32 %sampler1 to ptr addrspace(2) ; CHECK: %sampler2.ptrcast = inttoptr i32 %sampler2 to ptr addrspace(2) ; CHECK: call spir_kernel void @image_sampler.old(