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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG02b51e5316cd: [analyzer][solver] Redesign constraint ranges data structure (authored by vsavchenko). Changed prior to commit: https://reviews.llvm

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

steakhal added a comment. In D86465#2640958 , @vsavchenko wrote: > In D86465#2640954 , @steakhal wrote: > >> It's getting complicated then xD. I guess we should complement unittests >> with LIT tests? >> You can kn

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

vsavchenko added a comment. In D86465#2640954 , @steakhal wrote: > In D86465#2640875 , @vsavchenko > wrote: > >> This is not only a compiler feature, it also should be supported by the >> target architecture: >> h

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

steakhal added a comment. In D86465#2640875 , @vsavchenko wrote: > This is not only a compiler feature, it also should be supported by the > target architecture: > https://godbolt.org/z/ddjYYx9x6 It's getting complicated then xD. I guess we should comple

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

vsavchenko added a comment. In D86465#2637186 , @steakhal wrote: > Ah, I wanted to give it a go, but the bots caught an assertion failure for > the parent revision of this. See the details at the Bugzilla ticket >

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

steakhal added a comment. In D86465#2640765 , @vsavchenko wrote: > I do have a bit of a struggle here. This is a unit-test and thus should be > compiled for all of the supported architectures by all of the supported > compilers. > Is there a `__has_feat

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

vsavchenko added a comment. In D86465#2640687 , @steakhal wrote: > Given that it did not change any reports in our testbench it seems to be safe > to land it. > > It clearly improves the API significantly, so I'm not opposing. > Really good work @vsavchen

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

vsavchenko added a comment. In D86465#2637186 , @steakhal wrote: > Ah, I wanted to give it a go, but the bots caught an assertion failure for > the parent revision of this. See the details at the Bugzilla ticket >

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

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Given that it did not change any reports in our testbench it seems to be safe to land it. It clearly improves the API significantly, so I'm not opposing. Really good work @vsavchenko. PS:

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

steakhal added a comment. Ah, I wanted to give it a go, but the bots caught an assertion failure for the parent revision of this. See the details at the Bugzilla ticket . What is more concerning is that this patch resolves that assertion failure. I

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

vsavchenko updated this revision to Diff 331008. vsavchenko added a comment. Add minor fix in tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86465/new/ https://reviews.llvm.org/D86465 Files: clang/include/clang/StaticAnalyzer/Core/PathSens

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

vsavchenko added a comment. Oof, it took me quite some time to come back to this. I don't think that I'll be able to gather more performance-related data than I already did, so if it's OK with y'all, we can land it. @NoQ @steakhal @ASDenysPetrov ? Comment at: clang/include/cl

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

vsavchenko updated this revision to Diff 331005. vsavchenko marked 16 inline comments as done. vsavchenko added a comment. Rebase and address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86465/new/ https://reviews.llvm.org/D86465

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

vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:433-434 +if (To == MAX) { + Result.insert(Result.end(), What.begin(), What.end()); + return makePersistent(std::move(Result)); +} ASDenysPetro

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

vsavchenko added a comment. In D86465#2569403 , @ASDenysPetrov wrote: > @vsavchenko > Hi, I actually want this patch goes developing and be loaded. It's really the > worth one. Is it abandoned? Hi, nope. Just got postponed, I'll address the review com

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

ASDenysPetrov added a comment. @vsavchenko Hi, I actually want this patch goes developing and be loaded. It's really the worth one. Is it abandoned? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86465/new/ https://reviews.llvm.org/D86465 ___

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

ASDenysPetrov added a comment. My propositions for the function `negate`. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:421 + ContainerType Result; + Result.reserve(What.getMinValue() == MIN ? What.size() + 1 : What.size()); Here you

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

ASDenysPetrov added a comment. I didn't check correctness of each Factory function but if it passes all tests and improves performance, I think it's enough. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:77-78 + // + // * Range

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

ASDenysPetrov added a comment. Here is my five cents. I haven't done with the review yet. I'm gonna return to it a bit later. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:56 + bool operator==(const Range &RHS) const { return Imp

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

steakhal added a comment. Previously, I liked this. Now I love it! The benchmarks look promising as well. I think I can not convince you about not modifying iterators using `++`,`--`, etc. outside of //loop-expressions//. So be it :D Comment at: clang/include/clang/StaticAna

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

vsavchenko added a comment. In D86465#2238614 , @martong wrote: > These are really promising figures, nice work! (And the measurement stuff > itself is also a great addition, thanks for that!) Thanks 😊 > Not that it would matter much, but I was just won

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

vsavchenko updated this revision to Diff 288306. vsavchenko marked 21 inline comments as done. vsavchenko added a comment. Fix review remarks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86465/new/ https://reviews.llvm.org/D86465 Files: clang/

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

steakhal added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:118-123 +/// Create a new set with all ranges from both LHS and RHS. +/// Possible intersections are not checked here. +/// +/// Complexity

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

martong added a comment. In D86465#2238379 , @vsavchenko wrote: > Here are some benchmarking results. > In docker: > F12777445: tiny.png > F12777444: small.png > Natively on my Mac

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

vsavchenko added a comment. Here are some benchmarking results. In docker: F12777445: tiny.png F12777444: small.png Natively on my Mac (no memory measurements due to this issue

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

vsavchenko added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:110 + iterator end() const { return Impl->end(); } + size_t size() const { return Impl->size(); } + steakhal wrote: > I think it shoul

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

steakhal added a comment. This is a huge change, I've got pretty tired at the end of the review. I haven't checked the test-cases too deeply. I've found only minor typos, clarity enhancement opportunities etc. nothing serious. It was awesome to see how this code can be improved, even becoming c

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

vsavchenko added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:88 + // structure is preferred. + using ImplType = llvm::SmallVector; + vsavchenko wrote: > NoQ wrote: > > vsavchenko wrote: > > >

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

vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:119 + + return makePersistent(std::move(Result)); +} NoQ wrote: > Given that we're certain from the start that the container will be > persistent, can we save

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

vsavchenko added a comment. In 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

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

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: c

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

vsavchenko added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:88 + // structure is preferred. + using ImplType = llvm::SmallVector; + grandinj wrote: > Just curious - if they mostly contain 1

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

grandinj added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:88 + // structure is preferred. + using ImplType = llvm::SmallVector; + Just curious - if they mostly contain 1 or 2 elements, why i

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

vsavchenko created this revision. vsavchenko added reviewers: NoQ, dcoughlin, xazax.hun, Szelethus, ASDenysPetrov, steakhal. Herald added subscribers: cfe-commits, martong, Charusso, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, mgrang, rnkovacs, szepet, baloghadamsoftware. Herald added a proje