Skip to content

[SPV->LLVM] Fix global c/dtors type if SPV is from opaque type LLVM #2280

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Dec 27, 2023
Merged

[SPV->LLVM] Fix global c/dtors type if SPV is from opaque type LLVM #2280

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7bfb37
[SPV->LLVM] Fix global c/dtors type if SPV is from opaque type LLVM
wenju-he Dec 21, 2023
fa4372c
move the new code into transGlobalCtorDtors
wenju-he Dec 22, 2023
050a9c7
auto
wenju-he Dec 22, 2023
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
53 changes: 47 additions & 6 deletions lib/SPIRV/SPIRVReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1331,6 +1331,39 @@ void SPIRVToLLVM::transFunctionPointerCallArgumentAttributes(
}
}

/// When spirv is generated from LLVM IR with opapque pointer enabled and then
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: opaque

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/// the spirv is translated to LLVM IR with typed pointer, function pointer is
/// casted to i8* type in \p Ty and \p Initializer, which causes error in LLVM
/// IR verifier. E.g.
/// [1 x %0][%0 { i32 1, i8* bitcast (void ()* @ctor to i8*), i8* null }]
/// This function removes the cast so that LLVM IR is valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to better understand the scope of the problem when we get an opaque pointer type rather than function
type, and so interpret it further like a i8* pointer when reverse translate it back to LLVM. Does this behavior manifest itself for ctor/dtor only, and that is caused by special treatment of ctor/dtor case in other places of the source code base? Or this is rather a wider problem that is not only about ctor/dtor?

Copy link
Contributor Author

@wenju-he wenju-he Dec 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with your concern that this behavior could be a wider problem.
I was also thinking why this issue is exposed so late. My guess is that IGC who uses llvm-14 branch switched to use the open-source SPIRV-LLVM-Translator not long time ago, so some issues related to opaque types are not exposed yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ctor/dtor use function pointer. This problem here is kind of special that LLVM IR verifier specifically requires that the type of the second element should be a function type rather than i8* type, see https://github.com/llvm/llvm-project/blob/248fba0cd806a0f6bf4b0f12979f2185f2bed111/llvm/lib/IR/Verifier.cpp#L798

Indeed, the issue this PR addresses might also be related to cases that a function pointer is used in the code, e.g. as an indirect function call. With the second commit of this PR, I verified that SPV from opaque pointer in tests in https://github.com/KhronosGroup/SPIRV-LLVM-Translator/tree/main/test/extensions/INTEL/SPV_INTEL_function_pointers could be translated to LLVM IR with typed pointer using SPIRV-LLVM-Translator llvm_release_140 branch. Therefore, function pointer handling seems good.

Other than function pointer, I don't know if opaque types handling in SPIRV-LLVM-Translator llvm_release_140 branch has more issues. As @LU-JOHN mentioned, we might need the whole SPIRVTypeScavenger.

static void postProcGlobalCtorDtorTypeInit(Type *&Ty, Constant *&Initializer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The function is defined to be void, as if it would cause a side effect rather than convert one statement into another, as it actually does by reassigning an argument Initializer . It would require more time to understand the underlying logic comparing to line 1595 style of
Initializer = dyn_cast<Constant>(transValue(Init, F, BB, false));.
In my opinion, it's better to return a pointer than to change the *& argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

auto *InitArr = cast<ConstantArray>(Initializer);
unsigned NumEltsInArr = InitArr->getType()->getNumElements();
assert(NumEltsInArr && "array is empty");
auto *CS = cast<ConstantStruct>(InitArr->getAggregateElement(0u));
assert(CS->getType()->getNumElements() == 3 && "expect 3 elements in struct");
Copy link
Contributor

Choose a reason for hiding this comment

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

This new function is not added to validate, so I'm not sure you need new asserts here at all. It looks like the following logic can be just protected by if conditions, for example, to avoid adding implicit layer of validity checks into a new ad-hoc function that addresses ctor/dtor case. However, if other reviewers think that asserts are ok here and may remain as it is, I'd strongly suggest to consider better error messages, adding more context to the message to facilitate possible future testing/debugging efforts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assert is to make sure StTy->getElementType(2) and CS->getAggregateElement(2) below are valid and the assert aligns with assert at https://github.com/llvm/llvm-project/blob/248fba0cd806a0f6bf4b0f12979f2185f2bed111/llvm/lib/IR/Verifier.cpp#L799

I'd strongly suggest to consider better error messages

done

auto *Elt1 = CS->getAggregateElement(1);
if (!isa<ConstantExpr>(Elt1))
return;
auto *StTy = CS->getType();
auto *NewStTy = StructType::create(Ty->getContext(),
{StTy->getElementType(0),
Elt1->stripPointerCasts()->getType(),
StTy->getElementType(2)},
StTy->getName(), StTy->isPacked());
Ty = ArrayType::get(NewStTy, NumEltsInArr);
SmallVector<Constant *, 4> ArrElts;
for (unsigned I = 0; I < NumEltsInArr; ++I) {
auto *CS = cast<ConstantStruct>(InitArr->getAggregateElement(I));
ArrElts.push_back(ConstantStruct::get(
NewStTy, {CS->getAggregateElement(0u),
CS->getAggregateElement(1)->stripPointerCasts(),
CS->getAggregateElement(2)}));
}
Initializer = ConstantArray::get(cast<ArrayType>(Ty), ArrElts);
}

/// For instructions, this function assumes they are created in order
/// and appended to the given basic block. An instruction may use a
/// instruction from another BB which has not been translated. Such
Expand Down Expand Up @@ -1556,20 +1589,28 @@ Value *SPIRVToLLVM::transValueWithoutDecoration(SPIRVValue *BV, Function *F,
SPIRVBuiltinVariableKind BVKind;
if (BVar->isBuiltin(&BVKind))
BV->setName(prefixSPIRVName(SPIRVBuiltInNameMap::map(BVKind)));
bool IsCtorOrDtor = (BV->getName() == "llvm.global_ctors" ||
BV->getName() == "llvm.global_dtors");
if (Init && IsCtorOrDtor) {
Initializer = dyn_cast<Constant>(transValue(Init, F, BB, false));
postProcGlobalCtorDtorTypeInit(Ty, Initializer);
Copy link
Contributor

Choose a reason for hiding this comment

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

As in the comment to line 1340, it's not obvious that the function changes Initializer value. Initializer = would work better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's confusing to have this check separated from if-statement below, and to repeat the check for IsCtorOrDtor twice, here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the code into transGlobalCtorDtors

auto LVar = new GlobalVariable(*M, Ty, IsConst, LinkageTy,
/*Initializer=*/nullptr, BV->getName(), 0,
GlobalVariable::NotThreadLocal, AddrSpace);
auto Res = mapValue(BV, LVar);
if (Init)
Initializer = dyn_cast<Constant>(transValue(Init, F, BB, false));
else if (LinkageTy == GlobalValue::CommonLinkage)
if (Init) {
if (!IsCtorOrDtor)
Initializer = dyn_cast<Constant>(transValue(Init, F, BB, false));
} else if (LinkageTy == GlobalValue::CommonLinkage) {
// In LLVM, variables with common linkage type must be initialized to 0.
Initializer = Constant::getNullValue(Ty);
else if (BS == SPIRVStorageClassKind::StorageClassWorkgroup)
} else if (BS == SPIRVStorageClassKind::StorageClassWorkgroup) {
Initializer = dyn_cast<Constant>(UndefValue::get(Ty));
else if ((LinkageTy != GlobalValue::ExternalLinkage) &&
(BS == SPIRVStorageClassKind::StorageClassCrossWorkgroup))
} else if ((LinkageTy != GlobalValue::ExternalLinkage) &&
(BS == SPIRVStorageClassKind::StorageClassCrossWorkgroup)) {
Initializer = Constant::getNullValue(Ty);
}

LVar->setUnnamedAddr((IsConst && Ty->isArrayTy() &&
Ty->getArrayElementType()->isIntegerTy(8))
Expand Down
54 changes: 54 additions & 0 deletions test/transcoding/SPV_INTEL_function_pointers/global_ctor_dtor_opaque.spt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
119734787 65792 393230 22 0
2 Capability Addresses
2 Capability Linkage
2 Capability Kernel
2 Capability Int64
2 Capability Int8
2 Capability FunctionPointersINTEL
8 Extension "SPV_INTEL_function_pointers"
5 ExtInstImport 1 "OpenCL.std"
3 MemoryModel 2 2
3 Source 0 0
7 Name 14 "asan.module_ctor"
7 Name 15 "asan.module_ctor"
7 Name 20 "llvm.global_ctors"

9 Decorate 20 LinkageAttributes "llvm.global_ctors" Export
4 TypeInt 3 32 0
4 TypeInt 4 8 0
4 TypeInt 6 64 0
5 Constant 6 7 1 0
4 Constant 3 10 1
4 TypePointer 5 7 4
5 TypeStruct 2 3 5 5

4 TypeArray 8 2 7
4 TypePointer 9 7 8
2 TypeVoid 11
3 TypeFunction 12 11
4 TypePointer 13 7 12
4 ConstantFunctionPointerINTEL 13 15 14
5 SpecConstantOp 5 16 124 15
3 ConstantNull 5 17
6 ConstantComposite 2 18 10 16 17

4 ConstantComposite 8 19 18

5 Variable 9 20 7 19



5 Function 11 14 0 12

2 Label 21
1 Return

1 FunctionEnd

; RUN: llvm-spirv %s --to-binary -o %t.spv
; RUN: llvm-spirv -r %t.spv -o %t.bc
; RUN: llvm-dis %t.bc -o %t.ll
; RUN: FileCheck --input-file=%t.ll %s --check-prefix=CHECK-LLVM

; CHECK-LLVM: [[TY:%.*]] = type { i32, void ()*, i8* }
; CHECK-LLVM: @llvm.global_ctors = appending global [1 x [[TY]]] [[[TY]] { i32 1, void ()* @asan.module_ctor, i8* null }]