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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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!~

WDYT about moving introducing a generic "`SmallImmutableSet`" in `llvm/ADT` to back your implementation?



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:88
+  //     structure is preferred.
+  using ImplType = llvm::SmallVector<Range, 4>;
+
----------------
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.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:135
+    /// @{
+    RangeSet getSet(Range Origin);
+    RangeSet getSet(const llvm::APSInt &From, const llvm::APSInt &To) {
----------------
"But what about `setGet()`???" - some user, probably :)


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:119
+
+  return makePersistent(std::move(Result));
+}
----------------
Given that we're certain from the start that the container will be persistent, can we save a copy by directly asking the factory to allocate a fresh container?

Also this seems to be one of the cases where variable-sized small vectors would make sense.


================
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:
----------------
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}});
----------------
Why is it necessary to specify `this->` here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86465/new/

https://reviews.llvm.org/D86465