ymandel added a comment. In D153493#4450358 <https://reviews.llvm.org/D153493#4450358>, @sammccall wrote:
>> I realize the complexity is frustrating, but I don't see how that's related >> to the issue here. The complexity of the `JoinedStateBuilder` is caused by >> the desire to minimize copies. The multiple cases come directly from the >> algorithm itself, most particularly that we may or may not be in an initial >> state, and the previous block may or may not have an environment we care >> about. At least, that is the specific issue I'm concerned with. > > We have 9 state transitions (3 states x 3 operations). Each would be less > than half as complicated if it didn't have to deal with Lattice's join being > mutating and Environment's being non-mutating. > > I believe further simplifications are possible (essentially: back at the > callsite track `vector<const Env*> All` plus `deque<Environment> Owned` for > owned copies, special case `All.size() == 0` and `All.size() == 1` and > otherwise `joinVec(All)`. > However with Lattice's mutating join involved this no longer works, and after > addressing that it's no longer simpler. I'm pretty sure I agree on all of this, please document it somewhere (file an Issue in the github tracker?). FWIW, I think mutating join was a case of premature optimization gone bad, and ultimately the builtin lattice should not be built in at all -- it should exist outside the core and be just another client. >> "join with InitEnv" should reduce to the identity function, so if its not, >> that feels like a problem we're sweeping under the rug. > > Only in a mathematical sense - in reality humans need to read these SAT > systems, and we're using a solver that can't perform that reduction. I think we're talking past each other (and actually agreeing), sorry. To be clear, I don't mean that as a criticism of this change -- I think you've hit an important point and fixing it might make this simpler. I totally agree about SAT systems, and the solver, etc. But, I'm saying that it shouldn't come to that. I think the Environment join operation is broken if `join(InitEnv, E) != E`. I'm asking whether we can reduce (some) complexity by fixing that. From your earlier point, there are more problems, but I want to focus on this particular one in case solving it makes other things easier. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153493/new/ https://reviews.llvm.org/D153493 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
