This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Use `Strict` accessors in more places in Transfer.cpp.
ClosedPublic

Authored by mboehme on May 16 2023, 3:15 AM.

Details

Summary

This patch handles the straightforward cases. Upcoming separate patches will handle the cases that are more subtle.

This patch is part of the ongoing migration to strict handling of value categories (see https://discourse.llvm.org/t/70086 for details).

Depends On D150653

Diff Detail

Event Timeline

mboehme created this revision.May 16 2023, 3:15 AM
Herald added a project: Restricted Project. · View Herald Transcript
mboehme requested review of this revision.May 16 2023, 3:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 3:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mboehme edited the summary of this revision. (Show Details)May 16 2023, 3:16 AM
mboehme added reviewers: sammccall, ymandel, xazax.hun.
sammccall added inline comments.May 16 2023, 5:05 AM
clang/lib/Analysis/FlowSensitive/Transfer.cpp
138

the name "forward" isn't clear to me - if anything suggesting std::forward, but not assignment.

Maybe assignValue or copyValue, and aliasLocation for the storageloc variant?

That leaves "forwardValueOrStorageLocation", but that only has one callsite and part of the point of this refactoring is that we're best thinking about these cases explicitly, right? So could just inline the if.

mboehme added inline comments.May 16 2023, 5:24 AM
clang/lib/Analysis/FlowSensitive/Transfer.cpp
138

(Haven't made any changes yet because I'd like to discuss first.)

That leaves "forwardValueOrStorageLocation", but that only has one callsite and part of the point of this refactoring is that we're best thinking about these cases explicitly, right? So could just inline the if.

This function will end up being used in more places though. I have some draft patches that use it (for the comma operator, for example) but these aren't ready for review yet. But as I know the operation will be used elsewhere, it made sense to create an abstraction now (and we'll definitely need a name for it at some point).

I think the most generic of the names you suggest is copy, and I think it works reasonably well: copyValue(), copyStorageLocation(), copyValueOrStorageLocation(). WDYT?

ymandel accepted this revision.May 16 2023, 5:28 AM
ymandel added inline comments.
clang/lib/Analysis/FlowSensitive/Transfer.cpp
138

I see Martin's point in not wanting to use copy/assign, since the same value will be shared between the locations. but, I see why "forward" has multiple meanings. Maybe "share" or "propogate"?

751–752

just else if?

This revision is now accepted and ready to land.May 16 2023, 5:28 AM
mboehme added inline comments.May 16 2023, 6:09 AM
clang/lib/Analysis/FlowSensitive/Transfer.cpp
138

I like "propagate", since it conveys the same meaning that I was going for with "forward", but without the allusion to std::forward. I like it better than "copy" or "assign" because those also have specific meanings in C++, and "propagate" doesn't.

@sammccall WDYT?

sammccall added inline comments.May 16 2023, 6:21 AM
clang/lib/Analysis/FlowSensitive/Transfer.cpp
138

Propagate is fine, personally I like "share" better though because "sharing a location" is direct and "propagating a location" is abstract.

I don't think propagateValueOrStorageLocation() should exist though. I can deal with the new model where lvalues and rvalues are totally different things, or the old model where we pretend references are bizarro-pointers and lvalues and rvalues are basically the same. I don't think saving a few if statements is a good reason to mix the two. In the new model the function does two ~unrelated things.

mboehme updated this revision to Diff 522583.May 16 2023, 6:26 AM

Simplified else { if {} } to else if {}.

mboehme marked an inline comment as done.May 16 2023, 6:26 AM
mboehme added inline comments.
clang/lib/Analysis/FlowSensitive/Transfer.cpp
751–752

Done.

mboehme marked an inline comment as done.May 16 2023, 6:37 AM
mboehme added inline comments.
clang/lib/Analysis/FlowSensitive/Transfer.cpp
138

Propagate is fine, personally I like "share" better though because "sharing a location" is direct and "propagating a location" is abstract.

"Share" doesn't imply a direction ("from ... to ...") in the way that "propagate" does -- so if it's OK with you, I'd like to use "propagate".

I don't think propagateValueOrStorageLocation() should exist though. I can deal with the new model where lvalues and rvalues are totally different things, or the old model where we pretend references are bizarro-pointers and lvalues and rvalues are basically the same. I don't think saving a few if statements is a good reason to mix the two. In the new model the function does two ~unrelated things.

While most AST nodes operate either on prvalues or glvalues, there are some that can propagate either a prvalue or a glvalue. For example:

  • The conditional operator
  • The comma operator (for the RHS)
  • No-op casts

The operation I'm trying to express is "propagate a value of any category (but don't change its category)". I think this is a useful abstraction that corresponds to a concept in the language, but I'm happy to inline the code if you feel strongly about this.

xazax.hun accepted this revision.May 16 2023, 10:33 AM
xazax.hun added inline comments.
clang/lib/Analysis/FlowSensitive/Transfer.cpp
717–723

This operation is basically something like an RValue to LValue cast. I am not sure how many of these will we have, but it might be a good idea to look out if we want to create something like propagateAsLValue similar to propagateValue.

sammccall accepted this revision.May 17 2023, 6:19 AM
sammccall added inline comments.
clang/lib/Analysis/FlowSensitive/Transfer.cpp
138

Yeah, propagate is fine, and go ahead with the function, I don't think I feel strongly about it in the end.

propagate a value of any category

(For the record, I think "value of any category" is pretty hard to keep hold of now: it has no type (Value and StorageLocation are distinct) and no name (we're using "Value" to mean ~rvalue), which is why I find it hard to think about this operation)

mboehme updated this revision to Diff 523031.May 17 2023, 6:24 AM

Renamed forward... functions to propagate....

mboehme marked an inline comment as done.May 17 2023, 7:12 AM
mboehme added inline comments.
clang/lib/Analysis/FlowSensitive/Transfer.cpp
717–723

I'll keep an eye out for this, but it's plausible to me that "materialize temporary" is the only operation of this kind. ("Materialize temporary" essentially means "rvalue to lvalue", if you think about it.)

This revision was landed with ongoing or failed builds.May 21 2023, 11:51 PM
This revision was automatically updated to reflect the committed changes.
mboehme marked an inline comment as done.