ymandel accepted this revision. ymandel added a comment. This revision is now accepted and ready to land.
I don't want to block the progress you're making elsewhere and I think the concerns here are sufficiently localized to revisit another time. So, feel free to reify any suggestions/disagreements in FIXMEs and submit. In D153493#4449993 <https://reviews.llvm.org/D153493#4449993>, @sammccall wrote: >> Regarding the design. This looks like an optimal solution in terms of >> copying but introduces lifetime risks and complexity that I'm uncomfortable >> with. > > FWIW I agree with the complexity (not the lifetime risks). > I think it ultimately stems from having too many interacting types & > functions with complicated and inconsistent signatures & constraints > (TypeErasedDataflowAnalysisState, Environment, TypeErasedLattice, Lattice, > the various join functions, etc) so we resort to case-bashing. > However this system is difficult to make changes to, and getting consensus as > to what changes are good also doesn't seem easy. So I think we need to be > able to fix bugs within the existing design (where copies need to be avoided > ad-hoc). 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. >> There's a lot of flexibility here and I wonder if we can reduce that without >> (substantially?) impacting the amount of copying. Specifically, I wonder if >> we can have the builder *always* own an environment, which simplifies the >> handling of CurrentState and, generally, the number of cases to consider. It >> costs more in principle, but maybe negligible in practice? > > I don't understand the specific suggestion: > > - if we create the builder lazily, we're just moving handling this extra > state into the callsites > - if the builder owns a copy of initenv, its FC token will end up in the > result FC > - if the builder owns a copy of initenv and detects when it should discard > it, that just sounds like nullptr with extra steps > > can you clarify? I suppose I'm thinking along the lines of: > - if the builder owns a copy of initenv, its FC token will end up in the > result FC Can you expand on why this is a problem? That would help motivate the complexity. "join with InitEnv" should reduce to the identity function, so if its not, that feels like a problem we're sweeping under the rug. 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
