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

DRAFT: Fix for spir-v implicit address space cast #651

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions modules/compiler/spirv-ll/source/builder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ void spirv_ll::Builder::generateBuiltinInitBlock(spv::BuiltIn builtin,
default:
SPIRV_LL_ABORT("BuiltIn unknown");
}
}
}

bool spirv_ll::Builder::replaceBuiltinUsesWithCalls(
llvm::GlobalVariable *builtinGlobal, spv::BuiltIn kind) {
Expand All @@ -345,6 +345,7 @@ bool spirv_ll::Builder::replaceBuiltinUsesWithCalls(

while (!Worklist.empty()) {
auto *UI = Worklist.pop_back_val();
llvm::errs() << "CSD builtinGlobal " << builtinGlobal->getName() << " user : " << *UI << "\n";

// We may have addrspacecast constant expressions
if (auto *CE = dyn_cast<llvm::ConstantExpr>(UI)) {
Expand Down Expand Up @@ -488,13 +489,15 @@ void spirv_ll::Builder::replaceBuiltinGlobals() {
for (const auto &ID : module.getBuiltInVarIDs()) {
llvm::GlobalVariable *builtin_global =
llvm::cast<llvm::GlobalVariable>(module.getValue(ID));

llvm::errs() << "CSD replaceBuiltinGlobals:: builtin_global is " << builtin_global->getName() << "\n";
llvm::errs() << *(builtin_global->getParent());
// Erase the global and return early if it wasn't used.
if (builtin_global->use_empty()) {
builtin_global->eraseFromParent();
return;
}


// To generate the init block we need the type of the builtin and which
// builtin the variable was decorated with.
llvm::Type *varTy = builtin_global->getValueType();
Expand All @@ -518,8 +521,11 @@ void spirv_ll::Builder::replaceBuiltinGlobals() {
user_functions;

for (auto user : builtin_global->users()) {
llvm::errs() << "CSD user is " << *user << "\n";
llvm::Function *use_function =
llvm::cast<llvm::Instruction>(user)->getParent()->getParent();
llvm::errs() << "CSD builtin_global name is " << builtin_global->getName() << " Func is " << use_function->getName() << "\n";


auto iter = user_functions.find(use_function);

Expand Down
18 changes: 15 additions & 3 deletions modules/compiler/spirv-ll/source/builder_core.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4017,9 +4017,21 @@ llvm::Error Builder::create<OpBitcast>(const OpBitcast *op) {
SPIRV_LL_ASSERT_PTR(value);

llvm::Type *type = module.getLLVMType(op->IdResultType());
SPIRV_LL_ASSERT_PTR(type);

llvm::Value *result = IRBuilder.CreateBitCast(value, type);
llvm::Value *result = nullptr;
if (llvm::isa<llvm::PointerType>(value->getType()) &&
llvm::isa<llvm::PointerType>(type) &&
value->getType()->getPointerAddressSpace() != type->getPointerAddressSpace()) {
// We need to do an addrspacecast first as the address spaces are
// different. Create actual instructions as
// spirv_ll::Builder::replaceBuiltinGlobals() make assumptions about the
// user being an actual instruction, wherwas the irbuilder tries to fold
// it. Although that could be less brittle the solution would be to
// convert back to an instruction so just create as an instruction here.
//value = new llvm::AddrSpaceCastInst(value, type);
//IRBuilder.Insert(value);
value = IRBuilder.CreateAddrSpaceCast(value, type);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The effect of a bitcast is supposed to be that of a store to memory as one type, and a load from the same memory as another type. I could be wrong, but I believe in general an addrspacecast is not guaranteed to be bit-pattern-preserving and this could be a wrong translation, unless I am missing a guarantee that we make somewhere that it doesn't matter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://llvm.org/docs/LangRef.html#addrspacecast-to-instruction "(i.e. casting the result back to the original address space should yield the original bit pattern)." - are you sure it's not reasonable to do this? How else would we get the same effect?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Casting the result back may yield the original bit pattern, but in the different address space it may have a different bit pattern. Also from that link: "It can be a no-op cast or a complex value modification, depending on the target and the address space pair." I'm not sure what the best way to translate this is, one possibility is actually doing an alloca to have somewhere to store and load from, another possibility is it may be okay to do ptrtoint and then inttoptr to the other address space, there may yet be more options that I'm not seeing right now.

result = IRBuilder.CreateBitCast(value, type);
module.addID(op->IdResult(), op, result);
return llvm::Error::success();
}
Expand Down
1 change: 1 addition & 0 deletions modules/compiler/spirv-ll/test/spvasm/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2079,6 +2079,7 @@ set(SPVASM_FILES
op_spec_constant_op_fsub_float.spvasm
op_spec_constant_true_bool.spvasm
op_spec_constant_uint.spvasm
op_sycl_addrspacecast_bitcast.spvasm
op_switch_long.spvasm
op_switch_short.spvasm
op_switch_char.spvasm
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
; Copyright (C) Codeplay Software Limited
;
; Licensed under the Apache License, Version 2.0 (the "License") with LLVM
; Exceptions; you may not use this file except in compliance with the License.
; You may obtain a copy of the License at
;
; https://github.com/codeplaysoftware/oneapi-construction-kit/blob/main/LICENSE.txt
;
; Unless required by applicable law or agreed to in writing, software
; distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
; WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
; License for the specific language governing permissions and limitations
; under the License.
;
; SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

; RUN: %if online-spirv-as %{ spirv-as --target-env %spv_tgt_env -o %spv_file_s %s %}
; RUN: %if online-spirv-as %{ spirv-val %spv_file_s %}
; RUN: spirv-ll-tool -a OpenCL -b 64 -c GenericPointer -e SPV_KHR_linkonce_odr %spv_file_s | FileCheck %s

OpCapability Addresses
OpCapability Linkage
OpCapability Kernel
OpCapability Int64
OpCapability GenericPointer
OpCapability Int8
OpExtension "SPV_KHR_linkonce_odr"
%1 = OpExtInstImport "OpenCL.std"
OpMemoryModel Physical64 OpenCL
OpName %__spirv_BuiltInLocalInvocationId "__spirv_BuiltInLocalInvocationId"
OpName %_Z27__spirv_LocalInvocationId_zv "_Z27__spirv_LocalInvocationId_zv"
OpName %_Z28__spirv_GlobalInvocationId_xv "_Z28__spirv_GlobalInvocationId_xv"

OpDecorate %__spirv_BuiltInLocalInvocationId LinkageAttributes "__spirv_BuiltInLocalInvocationId" Import
OpDecorate %__spirv_BuiltInLocalInvocationId Constant
OpDecorate %__spirv_BuiltInLocalInvocationId BuiltIn LocalInvocationId
OpDecorate %__spirv_BuiltInLocalInvocationId Alignment 32

OpDecorate %__spirv_BuiltInGlobalInvocationId LinkageAttributes "__spirv_BuiltInGlobalInvocationId" Import
OpDecorate %__spirv_BuiltInGlobalInvocationId Constant
OpDecorate %__spirv_BuiltInGlobalInvocationId BuiltIn GlobalInvocationId
OpDecorate %__spirv_BuiltInGlobalInvocationId Alignment 32


OpName %_useLocalInvocationId "_useLocalInvocationId"
OpName %_useGlobalInvocationId "_useGlobalInvocationId"

%ulong = OpTypeInt 64 0
%ulong_16 = OpConstant %ulong 16
%uchar = OpTypeInt 8 0
%uint = OpTypeInt 32 0
%v3ulong = OpTypeVector %ulong 3
%_ptr_CrossWorkgroup_v3ulong = OpTypePointer CrossWorkgroup %v3ulong
%void = OpTypeVoid
%_ptr_CrossWorkgroup_uchar = OpTypePointer CrossWorkgroup %uchar
%574 = OpTypeFunction %ulong
%575 = OpTypeFunction %ulong
%403 = OpTypeFunction %ulong
%_ptr_CrossWorkgroup_ulong = OpTypePointer CrossWorkgroup %ulong
%__spirv_BuiltInLocalInvocationId = OpVariable %_ptr_CrossWorkgroup_v3ulong CrossWorkgroup
%__spirv_BuiltInGlobalInvocationId = OpVariable %_ptr_CrossWorkgroup_v3ulong CrossWorkgroup

OpDecorate %__spirv_BuiltInGlobalInvocationId LinkageAttributes "__spirv_BuiltInGlobalInvocationId" Import
OpDecorate %__spirv_BuiltInGlobalInvocationId Constant
OpDecorate %__spirv_BuiltInGlobalInvocationId BuiltIn GlobalInvocationId
OpDecorate %__spirv_BuiltInGlobalInvocationId Alignment 32

; CHECK: call spir_func i64 @_Z12get_local_idj
; CHECK: call spir_func i64 @_Z27__spirv_LocalInvocationId_zv

%_Z27__spirv_LocalInvocationId_zv = OpFunction %ulong None %574
%entry_11 = OpLabel
%623 = OpBitcast %_ptr_CrossWorkgroup_uchar %__spirv_BuiltInLocalInvocationId
%624 = OpInBoundsPtrAccessChain %_ptr_CrossWorkgroup_uchar %623 %ulong_16
%625 = OpBitcast %_ptr_CrossWorkgroup_ulong %624
%626 = OpLoad %ulong %625 Aligned 16
OpReturnValue %626
OpFunctionEnd

%_Z28__spirv_GlobalInvocationId_xv = OpFunction %ulong None %574
%entry_6 = OpLabel
%602 = OpBitcast %_ptr_CrossWorkgroup_ulong %__spirv_BuiltInGlobalInvocationId
%603 = OpLoad %ulong %602 Aligned 32
OpReturnValue %603
OpFunctionEnd


;%_useLocalInvocationId = OpFunction %void None %403
; %entry_10 = OpLabel
; %call5 = OpFunctionCall %ulong %_Z27__spirv_LocalInvocationId_zv
; OpReturnValue %call5
; OpFunctionEnd
;
;%_useGlobalInvocationId = OpFunction %void None %403
; %entry_12 = OpLabel
; %call6 = OpFunctionCall %ulong %_Z28__spirv_GlobalInvocationId_xv
; OpReturnValue %call6
; OpFunctionEnd

;%_Z27__spirv_LocalInvocationId_zv = OpFunction %ulong None %574
; %entry_11 = OpLabel
; %617 = OpInBoundsPtrAccessChain %_ptr_CrossWorkgroup_ulong %__spirv_BuiltInLocalInvocationId %uint_0 %uint_2
; %618 = OpLoad %ulong %617 Aligned 16
; OpReturnValue %618
; OpFunctionEnd
Loading