[SYCL] Fix 'move instead of copy' Coverity hits #17619
Conversation
sycl/source/detail/jit_compiler.cpp
Outdated
| @@ -1044,14 +1044,16 @@ jit_compiler::fuseKernels(QueueImplPtr Queue, | |||
| std::shared_ptr<detail::kernel_bundle_impl> KernelBundleImplPtr; | |||
| if (TargetFormat == ::jit_compiler::BinaryFormat::SPIRV) { | |||
| detail::getSyclObjImpl(get_kernel_bundle<bundle_state::executable>( | |||
| Queue->get_context(), {Queue->get_device()}, {FusedKernelId})); | |||
| Queue->get_context(), {Queue->get_device()}, | |||
There was a problem hiding this comment.
What is this code doing? Why do we need to call getSyclObjImpl here?
There was a problem hiding this comment.
Im not too sure, it does look strange that we dont seem to use the returned object impl after we get it. From the looks of it, this was introduced by @sommerlukas in #8747, would you mind commenting on this bit, thanks.
For now I just reverted the change.
There was a problem hiding this comment.
I think this is indeed dead code. Kernel fusion is currently not exercised since we removed the scheduler integration for it and we're currently planning a refactoring of this code.
I think you can just remove the entire if here.
There was a problem hiding this comment.
Ok, i've removed that if statement
There was a problem hiding this comment.
Changes on the JIT compiler look good to me, but I'd like @jchlanda to review the spec constant materialization bit of it.
|
@intel/llvm-reviewers-runtime friendly ping |
aelovikov-intel
dismissed
their stale review
Missed "if removal", need more time to look at it.
|
The Jenkins fail seems unrelated and I don't see how this PR could cause the issue. |
Avoid copying by:
Queueparameter ofcompleteSpecConstMaterializationpassed by a const reference. Also changesmaterializeSpecConstants,getTargetInfoandgetTargetFormatto accomodate this change.DefaultValuestring non const so that the move ctor is called automatically in twogetfunctions.