vsavchenko added a comment.
In D86465#2235289 <https://reviews.llvm.org/D86465#2235289>, @NoQ wrote:
> If the numbers are confirmed to be as good as what i've sneak-peeked so far,
> that should be pretty amazing. Also whoa, test coverage!~
I'll add the charts with performance in the next couple of days
> WDYT about moving introducing a generic "`SmallImmutableSet`" in `llvm/ADT`
> to back your implementation?
Oof, it is not really possible. First of all, existing `RangeSet` didn't
really ensure that ranges do not intersect (`add` is/was not checking for
that), and this is left for the user. Additionally, it is designed and
optimized specifically for `RangeSet` queries and operations. Like `intersect`
and `contains` in a more generic `Set` sense mean entirely different things.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:88
+ // structure is preferred.
+ using ImplType = llvm::SmallVector<Range, 4>;
+
----------------
NoQ wrote:
> vsavchenko wrote:
> > grandinj wrote:
> > > Just curious - if they mostly contain 1 or 2 elements, why is N == 4
> > > here?
> > Essentially, the main factor is **intuition** 😆
> > The main idea here is that allocations are the single most expensive thing
> > about ranges, so I want to avoid extra allocations not for 60% of range
> > sets, but for 90%. I still should definitely measure performance
> > difference for different values of N and gather some stats about the
> > lengths.
> Given that you pass those by pointers, i suspect you don't need to fix the
> size at all. In fact the small-size of the small vector is, by design, a
> //runtime// value and you can use `llvm::SmallVectorImpl *` instead, which
> can point to `SmallVector` of any small-size. This would allow you to
> allocate small vectors of variable length which you can take advantage of
> whenever you know the length in advance.
Sounds reasonable, I'll give it a shot!
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:281
+ //
+ // Shortcut: check that there is even a possibility of the intersection
+ // by checking the two following situations:
----------------
NoQ wrote:
> Thanks a lot for adding those comments. These methods were confusing as heck.
😊
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:206
+
+ this->checkNegate({{MIN, A}}, {{MIN, MIN}, {D, MAX}});
+ this->checkNegate({{MIN, C}}, {{MIN, MIN}, {B, MAX}});
----------------
NoQ wrote:
> Why is it necessary to specify `this->` here?
It is due to shenanigans in test fixtures implementation. I didn't find a
better workaround and other tests for other components also use `this->` as far
as I checked :-(
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86465/new/
https://reviews.llvm.org/D86465
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
