Sorry for the late review. This looks good to me, but I hope we will be able to undo it soon :)

LG!

LG!

Thanks! Sometimes I am wondering whether we actually need a full map for PRValues. E.g., once we processed a MaterializeTemporaryExpr, we now have a location for the value, and it feels like we represent the same thing twice, once in ExprToLoc + LocToVal and once in ExprToVal. It is probably not too bad and might be extra work to clean this up.

In D156658#4606400, @mboehme wrote:

We only do widening at loop heads, and this means that widening only affects locations and values that flow into the loop from the outside or from a previous loop iteration.

But convergence can also be blocked by locations and values that are only used within the loop body. If these change from loop iteration to loop iteration and we don't perform widening on them, we will conclude that the state of the loop body never converges.

Reverted in https://github.com/llvm/llvm-project/commit/8330116ebdcba58926f4f6fb402a4a6f010e8afc to unbreak build bot.

Mea culpa. Looks like I did not anticipate non-RPO worklists. Thanks for cleaning this up, looks good to me!

In D156658#4552965, @mboehme wrote:

I've investigated this in more detail. Unfortunately, it turns out that it's not quite as simple as just implementing widening on ExprToLoc.

One of the reasons for this is that we only apply widening at loop heads, but the expressions that are "blocking" convergence may be contained in a block that is not a loop head.

It looks as if, instead, what we should be doing to improve convergence in a sound manner is to implement widening for ExprToLoc. I'll submit a corresponding patch shortly.

Thanks, looks good to me!

Sounds good to me. I believe this will make check author's lives easier.

LGTM!

I did not do a thorough review checking every line, but I read the design paper and skimmed through this patch. Love the direction, and I am OK with landing this as is.

Thanks!

In D155694#4514322, @tomasz-kaminski-sonarsource wrote:

Updating this tests illustrated that we are missing a lot of coverage for the objects with trivial destructors, but I think this should be addressed as a separate patch.

LG, thanks!

Thanks! Looks good to me.

Thanks!

I love it, I think we can land this as is. If there are further comments, we can address those in follow-up PRs.

Overall looks good to me. I think the changes to the notes already make the analyzer more useful so there are some observable benefits to this patch.

Could you rebase this patch to the latest tip of tree?

Thanks!

Nice, looks like this change did catch some unintentional copies! Already paying dividends :)

In D153491#4445051, @ymandel wrote:

In D153491#4443704, @xazax.hun wrote:

This sounds extremely error-prone to me. In case copying the analysis state has side effects like this, I would argue we want such operations to be really explicit. What do you think?

Can you expand on this concern? Are you referring to the removal of the unintended copy (ie. this patch), or raising a concern about the how the underlying system handles copies altogether?

Is there a measurable perf cost for this determinism?

Should we have some documentation or tooltip explaining the users what the meaning of those names are?