Shared: Use sourceBoundedFastTC in TypeTracking
#20370
Conversation
MathiasVP
force-pushed
the
source-bounded-fast-tc-in-typetracking
branch
from
09ff926 to
efa5766
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the TypeTracking implementation by using sourceBoundedFastTC for transitive closure computation instead of manual recursion. The change aims to reduce high iteration counts observed in C/C++ projects by leveraging a Higher-Order Predicate (HOP) for better performance.
Key Changes
- Replaced manual recursion in
standardFlowsTowithsourceBoundedFastTCtransitive closure - Introduced
simpleLocalSmallStepPluspredicate using the optimized transitive closure - Modified
isLocalSourceNodepredicate to support the new approach
shared/typetracking/codeql/typetracking/internal/TypeTrackingImpl.qll
Outdated
Show resolved
Hide resolved
MathiasVP
force-pushed
the
source-bounded-fast-tc-in-typetracking
branch
from
efa5766 to
eae9a6f
Compare
| } | ||
|
|
||
| private predicate simpleLocalSmallStepPlus(Node localSource, Node dst) = | ||
| sourceBoundedFastTC(simpleLocalSmallStep/2, isLocalSourceNode/1)(localSource, dst) | ||
|
|
||
| cached |
There was a problem hiding this comment.
Isn't it better to cache simpleLocalSmallStepPlus, which will persist the underlying FastTC structure instead of the materialized relation? standardFlowsTo then needs to be non-cached and pragma[inline]ed. See also
There was a problem hiding this comment.
Uh, that's a good idea. I didn't actually know that caching a fastTC didn't just cache the materialized relation. I've fixed this in bd7e20d
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
shared/typetracking/codeql/typetracking/internal/TypeTrackingImpl.qll
Fixed
Show resolved
Hide resolved
MathiasVP
force-pushed
the
source-bounded-fast-tc-in-typetracking
branch
from
4dd45dc to
bd7e20d
Compare
MathiasVP
force-pushed
the
source-bounded-fast-tc-in-typetracking
branch
from
bd7e20d to
3aee4a8
Compare
shared/typetracking/codeql/typetracking/internal/TypeTrackingImpl.qll
Outdated
Show resolved
Hide resolved
| @@ -70,6 +70,10 @@ module TypeTracking<LocationSig Location, TypeTrackingInput<Location> I> { | |||
|
|
|||
| private class ContentOption = ContentOption::Option; | |||
|
|
|||
| private predicate isLocalSourceNode(LocalSourceNode n) { | |||
There was a problem hiding this comment.
This is a small helper for Gah, never mind, it's of course also input to the TC.standardFlowsTo. I think it should be moved down, so it appears directly before standardFlowsTo. Also, check whether the RA is affected by manually inlining this - if not, then that simplifies things.
…mpl.qll Co-authored-by: Anders Schack-Mulligen <[email protected]>
|
:til: when you fix a Code Scanning warning the conversation just ... disappears? I've made |
We've seen quite high iteration counts for this predicate in C/C++ on some projects. I don't see why we shouldn't use a HOP here 🤷
In particular, this significantly reduces the analysis time on https://github.com/openvinotoolkit/oneDNN when running with
--ram=6000(which we saw in the 2.23.0 upgrade)