[PATCH] D86465: [analyzer][solver] Redesign constraint ranges data structure

[Valeriy Savchenko via Phabricator via cfe-commits]

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