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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
