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

sammccall closed this revision. sammccall added a comment. Herald added a subscriber: wangpc. This relanded as 1e010c5c4fae43c52d6f5f1c8e8920c26bcc6cc7 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

sammccall added a comment. Thanks! I'll land this to eliminate the variables, and follow up with trying to simplify join(). In D153493#4450440 , @ymandel wrote: > In D153493#4450358 , @sammccall > wrote: > >>>

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

ymandel added a comment. In 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

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

sammccall added a comment. In D153493#4450191 , @ymandel wrote: > 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

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

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

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

sammccall updated this revision to Diff 534691. sammccall added a comment. address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153493/new/ https://reviews.llvm.org/D153493 Files:

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

sammccall marked an inline comment as done. sammccall added a comment. > 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

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

ymandel added a comment. Just FYI (since you mentioned it in your description): LatticeEffect is vestigial and should be removed. It's now only used (properly) for widening. Regarding the design. This looks like an optimal solution in terms of copying but introduces lifetime risks and

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

sammccall added a comment. Sorry, going to **revert**, review would still be appreciated Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153493/new/ https://reviews.llvm.org/D153493 ___ cfe-commits

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

sammccall added a comment. Sorry, I got confused between branches and landed this one without approval Going to review, review would still be appreciated Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153493/new/ https://reviews.llvm.org/D153493

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

cor3ntin added a comment. In D153493#4448851 , @sammccall wrote: > Sorry, this was a mid-air collision with f2123af1e7d75 > , fixed > in 1adc5a781faa >

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

Sorry, this was a mid-air collision with f2123af1e7d75, fixed in 1adc5a781faa On Mon, Jun 26, 2023 at 4:23 PM Corentin Jabot via Phabricator < revi...@reviews.llvm.org> wrote: > cor3ntin added a comment. > > This seem to produce build failures > > >

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

cor3ntin added a comment. This seem to produce build failures /home/cor3ntin/dev/compilers/LLVM/llvm-project/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp:50:27: error: calling a private constructor of class 'clang::dataflow::Environment' Environment Env =

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

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGae54f01dd8c5: [dataflow] avoid more accidental copies of

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

sammccall added a comment. BTW i thought this was just nice for debugging and saving copying a few maps, but Dmitri says that the way even trivial indirections like `(FC1 = FC2)`, `(FC2 = ...)` are expressed in the SAT solver means they can add significant cost there too... Repository: rG

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

sammccall added reviewers: ymandel, xazax.hun. sammccall added a comment. The idea with the change to Environment::join() is that the current signature forces the base environment to be mutable, but this sometimes forces the caller to make a copy, and the implementation actually copies

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

sammccall updated this revision to Diff 533463. sammccall added a comment. refactor, restore comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153493/new/ https://reviews.llvm.org/D153493 Files:

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

sammccall created this revision. Herald added subscribers: martong, xazax.hun. Herald added a reviewer: NoQ. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This is clunky but greatly improves