sammccall added a comment. Thanks! I'll land this to eliminate the variables, and follow up with trying to simplify join().
In D153493#4450440 <https://reviews.llvm.org/D153493#4450440>, @ymandel wrote: > 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 Thanks - I will file a bug if it turns out not to be trivial, but I think it probably is trivial to fix so I'll give that a shot first. > and ultimately the builtin lattice should not be built in at all -- it should > exist outside the core and be just another client. Yeah, this looks like a bigger change :-) >>> "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. Ah, got it, thanks! I think if we can change Environment (e.g. dropping FC tokens) so this just falls out, and it makes sense for other reasons, that'd be great. I'm not a big fan of special-casing InitEnv somehow though. 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
