[PATCH] D106136: [Analyzer][solver] Fix equivalence class invariant violation in removeDeadBindings

[Gabor Marton via Phabricator via cfe-commits]

martong abandoned this revision.
martong added a comment.

In D106136#2883707 <https://reviews.llvm.org/D106136#2883707>, @vsavchenko wrote:

> In D106136#2883100 <https://reviews.llvm.org/D106136#2883100>, @martong wrote:
>
>> Thanks for taking your time to take a look. And I accept your statements. Nevertheless, let me explain my motivation.
>>
>> I thought that a class is trivial if it has only one member. And it seemed perfectly logical to not store equivalence classes with one member. I.e `a` equals to `a` does not hold any meaningful information it just takes precious memory. When we add a new constraint we take careful steps to avoid adding a new class with one member. However, when remove dead kicks in, suddenly we end up having classes stored with one member, which is a somewhat inconsistent design IMHO. Perhaps some better documentation could have helped.
>
> Representative symbol gives its equivalence class an ID.  We use this ID for everything, for comparing and for mapping.  Since we live in a persistent world, we can't just change this ID at some point, it will basically mean that we want to replace a class with another class.  So, all maps where this class participated (constraints, class, members, disequality) should remove the old class and add the new class.  This is a huge burden.  You need to carefully do all this, and bloat existing data structures.
>
> Trivial class is an optimization for the vast majority of symbols that never get into any classes.  We still need to associate constraints with those.  This is where implicit Symbol to Class conversion comes in handy.
>
> Now let's imagine that something else is merged into it.  We are obliged to officially map the symbol to its class, and the class to its members.  When this happens, it goes into the data structure FOREVER.  And when in the future, with only one member, instead of keeping something that we already have from other versions of the data structure, you decide to remove the item from the class map, you actually use more memory when it was there!  This is how persistent data structures work, different versions of the same data structure share data.  And I'm not even talking here about the fact that you now need to replace the class with one ID with the class with another ID in every relationship.
>
> You can probably measure memory footprint in your example and see it for yourself.

Yeah, makes sense. Thanks for taking more time for further explanations. Still, IMHO, the definition of a **trivial class** needs a clear written documentation in the code itself, so other developers can easier understand and maintain this code. I am going to create an NFC patch that summarizes this discussion in a comment attached to the `isTrivial` function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106136