Do not crash on SPIR-V BuiltIns declared narrower than size_t - #438
Draft
pvelesko wants to merge 2 commits into
Draft
Do not crash on SPIR-V BuiltIns declared narrower than size_t#438pvelesko wants to merge 2 commits into
pvelesko wants to merge 2 commits into
Conversation
A module may legally declare LocalInvocationId and friends as a vector of
32-bit ints under Physical64, so the translator emits
call spir_func i32 @_Z32__spirv_BuiltInLocalInvocationIdi(i32 0)
while BiFModule defines that same symbol returning size_t. Itanium mangling
does not encode the return type, so linking BiF replaces the declaration and
leaves a call whose FunctionType disagrees with its callee.
spirv_builtin_narrow_return_type.ll runs BIFTransforms over such a call and
checks that it comes out rebuilt against the real i64 callee signature with the
result truncated back to i32, i.e. exactly the IR the module would have had if
it had declared the builtin at size_t width.
BuiltInLocalInvocationId_32bit.spvasm and
BuiltInLocalInvocationId_32bit_subgroup_shuffle.spvasm are end-to-end ocloc
tests for the same module shape. Without the following commit
getCalledFunction() returns null for the mismatched call, so the builtin's
implicit arguments are never propagated to the kernel, AddImplicitArgs installs
a null call operand and ocloc dies with SIGSEGV and no diagnostic.
All three fail without the following commit.
Signed-off-by: Paulius Velesko <pvelesko@pglc.io>
The SPIR-V spec types LocalInvocationId, GlobalInvocationId, GlobalSize,
WorkgroupSize, NumWorkgroups and friends as vectors of size_t, but a module
may legally declare them narrower, e.g.
OpDecorate %id BuiltIn LocalInvocationId
%v3uint = OpTypeVector %uint 3
%id = OpVariable %ptr_v3uint Input
under Physical64. The translator then emits
call spir_func i32 @_Z32__spirv_BuiltInLocalInvocationIdi(i32 0)
while BiFModule defines that function returning size_t, i.e. i64. Itanium
mangling does not encode the return type of a non-template function, so the
two are the same symbol: the definition silently replaces the declaration
when BiF is linked in, and under opaque pointers not even a bitcast is left
behind to mark the spot.
What remains is a call whose FunctionType disagrees with its callee.
CallBase::getCalledFunction() type-checks, so it returns null for such a
call and every consumer that asks that question treats it as indirect:
alwaysinline never fires, and llvm::CallGraph routes the edge to the
external node so BuiltinCallGraphAnalysis never propagates the builtin's
implicit arguments (LOCAL_ID_X/Y/Z) up to the kernel. AddImplicitArgs does
not go through getCalledFunction(), so it still rewrites the call site, and
its lookup of the implicit argument on the parent kernel then misses;
DenseMap::operator[] hands it a null Value* to use as a call operand. The
result is a call with null operands whose return value is also RAUW'd into
uses of the wrong type, and the next pass to walk those operands
dereferences null. ocloc dies with SIGSEGV and no diagnostic, on every
platform.
Where the builtin happens to need no implicit argument the kernel is
missing, WorkgroupId and GlobalOffset, there is no null operand and no
crash. Those are silently miscompiled instead: the i64 results are
legalized into lo/hi i32 pairs and the resulting six-element vector is
bitcast back down to three, so the components the kernel stores are not the
components it asked for. Which wrong values come out is platform dependent,
since it depends on how the legalized pair is laid out for that target; on
dg2 the kernel stores [x, 0, y] and drops z.
Repair the call sites in BIImport, right after BiF linking and before
removeFunctionBitcasts, by rebuilding them against the real callee
signature and converting the result back to the width the module declared.
That reproduces exactly the IR the module would have had if it had declared
the builtin at size_t width, so everything downstream, inlining included,
takes its normal path.
removeFunctionBitcasts cannot be extended to cover this. Its repair
strategy is to clone the callee body into a new function carrying the call
site's type, which for a return type mismatch would put a `ret i64` inside
a function declared to return i32. Only differing integer return types are
handled here; parameter mismatches are still left to removeFunctionBitcasts.
Fixes intel#395
Signed-off-by: Paulius Velesko <pvelesko@pglc.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#395 was closed because the input is out of spec: under
Physical64the OpenCL SPIR-V Environment spec requiresLocalInvocationIdto be a vector of 64-bit integers, and the module declares it 32-bit. Agreed on the input. This PR is about what IGC does with it, which today is aSIGSEGVwith no output (exit 139 on dg2, rpl-s, bmg-g21 and pvc) rather than a diagnostic.Mechanism: BiFModule defines
__spirv_BuiltInLocalInvocationIdreturning i64 while the module declares it returning i32. Itanium mangling omits the return type, so linking BiF silently replaces the declaration with the i64 definition and leaves calls whoseFunctionTypedisagrees with their callee.getCalledFunction()type-checks and returns null for those, they read as indirect calls, the builtin's implicit arguments (LOCAL_ID_X/Y/Z) are never propagated to the kernel, andAddImplicitArgsinstalls a null call operand. For builtins that need no implicit argument (WorkgroupId,GlobalOffset) there is no crash; the i64 result is legalized and bitcast into narrow lanes, which is a silent miscompile.fixCallsWithMismatchedReturnType()inBIImport::run()rebuilds such calls against the linked-in definition and converts the result to the declared width. Every builtin reachable here is an unsigned id or size, so zext is correct on widening. Parameter mismatches stay withremoveFunctionBitcasts().Marked draft for one decision: this makes IGC accept the out-of-spec module silently, which is the behaviour the tests lock in. If rejecting it with an error at the same point is preferred, I will change it to that; either way the null dereference goes away.
Tests: one
check-igctest on the repaired IR, and two ocloc.spvasmtests (REQUIRES: spirv-as) that exit -11 on master and printBuild succeeded.with this change.Related: #395