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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
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
----------------
steakhal wrote:
> Emphasize what you mean by //relaxation//. You meant probably something like //not replacing the complex symbol, just adding the simplified version to the class//.
ok


================
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
----------------
steakhal wrote:
> nor memory consumption
ok


================
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);
----------------
steakhal wrote:
> Could you please elaborate on this in the review? I don't get the reasoning. I might miss something.
It's not a reasoning, just a guessing. Another reason could be that simply the time we spend here is negligible compared to the time we spend e.g. in removeDeadBindings. (Actually,  removeDeadBindings is very hot and takes roughly 15-20 % of the runtime.) Perhaps I could do a measurement with a sampling profiler to be able to provide a precise explanation to this. But I think the point is that the runtime/memory consumption do not grow and the concrete reason is secondary.

However, you are right, I should not get into guessing here, I'll just remove this sentence.


================
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:   {
----------------
steakhal wrote:
> Please try to omit unnecessary changes.
I've made this change intentionally, so all hunks for each simplification steps are indented similarly. E.g., after the `CHECK-NEXT:` string comes 3 space characters until we the `{` character, see L16 and L34 and L51. Actually, I've made an indentation error in a previous patch, which I though I can correct now.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114619