[PATCH] D114619: [Analyzer][solver] Do not remove the simplified symbol from the eq class

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I see. Simplification is always good. Let's measure and compare the runtime characteristics before moving forward.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2226
+      //
+      // Empirical measurements show that if we relax assumption G then the
+      // runtime does not grow noticeably. This is most probably because the
----------------
Emphasize what you mean by //relaxation//. You meant probably something like //not replacing the complex symbol, just adding the simplified version to the class//.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2227
+      // Empirical measurements show that if we relax assumption G then the
+      // runtime does not grow noticeably. This is most probably because the
+      // cost of removing the simplified member is much higher than the cost of
----------------
nor memory consumption


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2228-2229
+      // runtime does not grow noticeably. This is most probably because the
+      // cost of removing the simplified member is much higher than the cost of
+      // simplifying the symbol.
       State = reAssume(State, ClassConstraint, SimplifiedMemberVal);
----------------
Could you please elaborate on this in the review? I don't get the reasoning. I might miss something.


================
Comment at: clang/test/Analysis/symbol-simplification-disequality-info.cpp:15-26
+  // CHECK:      "disequality_info": [
+  // CHECK-NEXT:   {
+  // CHECK-NEXT:     "class": [ "((reg_$0<int a>) + (reg_$1<int b>)) + (reg_$2<int c>)" ],
+  // CHECK-NEXT:     "disequal_to": [
+  // CHECK-NEXT:       [ "reg_$3<int d>" ]]
+  // CHECK-NEXT:   },
+  // CHECK-NEXT:   {
----------------
Please try to omit unnecessary changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114619