[PATCH] D153493: [dataflow] avoid more accidental copies of Environment

Mon Jun 26 13:48:52 PDT 2023

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