[PATCH] D150653: [clang][dataflow] Add `Strict` versions of `Value` and `StorageLocation` accessors.

Tue May 16 10:14:34 PDT 2023

ymandel added a comment.

In D150653#4346025 <https://reviews.llvm.org/D150653#4346025>, @mboehme wrote:

> In D150653#4345771 <https://reviews.llvm.org/D150653#4345771>, @ymandel wrote:
>
>> I'm a little confused by one of the points in the description:
>>
>>> `setValueStrict()`: The RFC proposes that this should always create the same StorageLocation for a given Value, but, in fact, the transfer functions that exist today don't guarantee this; almost all transfer functions unconditionally create a new `StorageLocation` when associating an expression with a `Value`.
>>
>> Since values are immutable, we (intentionally) share them between different storage locations. So, it makes sense that a single value could be associated with multiple storage locations. Now, there's a bug in that values aren't really immutable since you can update the fields, so we have a false sharing bug in the framework, but maybe that only applies to glvalues?
>
> Yes, values are and should definitely be shared between different storage locations.
>
> The comment in the RFC is specifically in the context of the "fake" storage locations that we create for prvalues (since `setValueStrict()` only deals with prvalues). I call these storage locations "fake" because a prvalue doesn't actually have a storage location. During the review of the RFC, one of the reviewers asked whether we should guarantee that a given `Value`, when associated with a prvalue `Expr`, should always use the same "fake" storage location, instead of creating a new storage location every time. The idea was that this might be required for convergence. This argument made sense to me at the time, but having looked at what the existing code does, almost every place that associates a `Value` with a prvalue expression unconditionally create a new "fake" storage location. So this implies to me that creating durable fake storage locations isn't necessary.
>
> Does this make sense?

Yes, makes sense.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150653/new/

https://reviews.llvm.org/D150653