561847 Variable copied when it could be moved in kernel.cpp
Maintainers usually reply within 2 days
Nobody has claimed this yet.
Assessment
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Newbie friendliness
- 72/100
Research direction
Start at the return statement in sycl/source/kernel.cpp around lines 246-256, where Queue is passed by value to impl->queryMaxNumWorkGroups. The other three call sites in sycl/source/detail/kernel_impl.hpp (around lines 422-444) already wrap the queue in std::move, so the fix is to match them. Done when that call uses std::move(Queue) and the SYCL build and existing tests still pass; the device-parameter refactor described in the issue is optional and larger.
Written by the indexing model from the issue text.
Description
Describe the bug
250 impl->getAdapter().call_nocheck<sycl::detail::UrApiKind::urKernelSetExecInfo>(
251 impl->getHandleRef(), UR_KERNEL_EXEC_INFO_CACHE_CONFIG,
252 sizeof(ur_kernel_cache_config_t), nullptr, &(ExecInfo.CacheConfig));
253 sycl::queue Queue{Dev};
254 return impl->queryMaxNumWorkGroups<Dimensions>(
CID 561847: (#undefined of undefined): Variable copied when it could be moved (COPY_INSTEAD_OF_MOVE)
copy_constructor_call: Queue is passed-by-value as parameter to queryMaxNumWorkGroups, when it could be moved instead.[show details]
Use std::move(Queue) instead of Queue.
To reproduce
Static analysis finding, reported by the Coverity Scan build of intel/llvm. No
runtime reproducer — the effect is a redundant refcount operation, not incorrect
behaviour.
Environment
N/A — static analysis finding, backend independent.
Additional context
The finding is valid. kernel_impl::queryMaxNumWorkGroups
takes its queue by value, and Queue is a local whose last use is this call —
the call is the return statement. So the copy constructor runs for nothing:
an atomic increment on the shared_ptr<queue_impl> plus the matching decrement
when Queue goes out of scope.
The three other call sites of the same function already use the moving form, at
kernel_impl.hpp:422,
:433
and :444,
where the queue-based ext_oneapi_get_info overloads forward with
std::move(Queue). kernel.cpp:255 is the one site of four that doesn't:
return impl->queryMaxNumWorkGroups<Dimensions>(
std::move(Queue), Range, Bytes + ExecInfo.DynamicWorkGroupMem);
There is a larger inefficiency at the same spot, which the one-line fix leaves
in place. The only use of the queue in the callee is
kernel_impl.hpp:383:
auto Device = Queue.get_device();
Everything afterwards uses only Device, including the
exceedsOccupancyResourceLimits fallback. Meanwhile kernel::queryMaxNumWorkGroups
already has the device — it is the Dev parameter — and line 253 constructs a
full sycl::queue from it just so the callee can unwrap it again. That
construction creates a queue_impl and a backend queue handle, i.e. a
urQueueCreate/urQueueRelease round trip per query call, which is far more
expensive than the copy Coverity flagged. It also gives an informational query a
side effect and an unrelated failure mode, since queue construction can throw for
reasons that have nothing to do with the query.
A cleaner fix would be to change kernel_impl::queryMaxNumWorkGroups to take a
const device & and have the three queue-based callers pass
Queue.get_device(), which removes the synthetic queue entirely and makes the
copy question moot. That is ABI-safe: the function is a template defined in a
detail header with no explicit instantiations of its own, so only internal
callers change and the exported instantiations of
kernel::queryMaxNumWorkGroups keep their signatures.
Either fix resolves the CID; the second also removes the queue construction.
AI (Claude Code): the finding is from the Coverity scan; the analysis and suggested fixes above are the agent's.
- Dominant language
- LLVM
- Stars
- 1.5k
- Forks
- 869
- Avg merge
- 3d 1h
- Merged PRs (30d)
- 194
Getting set up
- No Dockerfile or Docker Compose file
- No pull request template
- Read the contributing guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from intel/llvm
-
bug hip
Difficulty 2/5 1-3 hours Newbie friendliness 67/100
intel/llvm#23531 · 1 comment ·
Maintainers usually reply within 2 days
-
bug Coverity
Difficulty 1/5 Under an hour Newbie friendliness 78/100
Maintainers usually reply within 2 days
-
Coverity
Difficulty 1/5 Under an hour Newbie friendliness 82/100
Maintainers usually reply within 2 days
-
confirmed
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
intel/llvm#23011 · 3 comments ·
Maintainers usually reply within 2 days
-
confirmed
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
intel/llvm#23008 · 2 comments ·
Maintainers usually reply within 2 days
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
Fei2411/DeepSeek-Bot#634 ·
-
:Search Relevance/Vectors >test-failure needs:risk Team:Search Relevance
Difficulty 2/5 1-3 hours Newbie friendliness 61/100
elastic/elasticsearch#163668 · 3 comments · 1 reaction ·
Maintainers usually reply within 1 day
-
Handler accepts zero/negative NNCP_MAX_RETRIES and NNCP backoff values, stalling policies or hot-loopingPossibly taken @NopeCharon claimed this today. Open
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
nmstate/kubernetes-nmstate#1604 ·
-
bug
Difficulty 2/5 1-3 hours Newbie friendliness 70/100
-
chore(worker): five module header comments describe the code from before later issues changed itOpenarea:dev U2-later
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
bentleypark/aiwatch#1824 ·
Maintainers usually reply within 1 day