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

[Sam McCall via Phabricator via cfe-commits]

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