This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Add simplistic constant-propagation analysis.
ClosedPublic

Authored by ymandel on Dec 14 2021, 9:20 AM.

Details

Summary

Adds a very simple constant-propagation analysis for demo and testing purposes.

Also, fixes two (small) bugs in the testing framework code.

Diff Detail

Unit TestsFailed

Event Timeline

ymandel created this revision.Dec 14 2021, 9:20 AM
ymandel updated this revision to Diff 394325.Dec 14 2021, 11:20 AM

fix potential null pointer deref.

ymandel updated this revision to Diff 394417.Dec 14 2021, 5:08 PM

Reworks analysis to use bottom, fixes two small bugs in testing framework.

ymandel updated this revision to Diff 394549.Dec 15 2021, 6:24 AM

revised bottom representation and updated tests

ymandel edited the summary of this revision. (Show Details)Dec 15 2021, 6:26 AM
ymandel added reviewers: gribozavr2, sgatev, xazax.hun.
ymandel published this revision for review.Dec 15 2021, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2021, 6:33 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ymandel updated this revision to Diff 394566.Dec 15 2021, 7:34 AM

Coalesce all CP files into one test file.

ymandel updated this revision to Diff 394568.Dec 15 2021, 7:36 AM

updated file header

sgatev added inline comments.Dec 15 2021, 8:05 AM
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
70

Should this be a const ref? Same below.

98

Why not put the definitions below also in the anonymous namespace?

120

What do you think about inlining this in ConstantPropagationAnalysis::transfer? We don't expect to call it from elsewhere, right?

161

Maybe call this S to disambiguate from the name of the type?

162

Should this be a const ref?

181

Maybe call this HasConstVal if you'd like to keep this short?

198

Is this necessary?

218

Capitalize - Code? Or maybe inline this in the function call? Same below.

246

Maybe add tests for assignment from a function call (e.g. int target = foo();) and assignment from an arithmetic expression (e.g. int target = 1 + 2;)? Both should result in IsUnknown() state, right?

343

Add internal checks, similar to those in SameAssignmentInBranches?

This demo looks cool. While I know this is a demo, I think it misses some important features, namely invalidating the value when we take the address of a variable or pass it to a function by non-const reference.
I'm fine with not supporting those cases, but I'd love to see a comment about that. Just in case someone wants to use this analysis as a basis for something in production :)

clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
100

Do we need an actual array? I think this would copy "var" into a global array. Alternatively, using static constexpr char* kVar= "var", we would just refer to the string in the read only memory. (Or maybe both would behave the same because compiler optimizations?)

ymandel updated this revision to Diff 394594.Dec 15 2021, 9:53 AM
ymandel marked an inline comment as done.

Responds to reviewer comments.

ymandel marked 10 inline comments as done.Dec 15 2021, 10:03 AM
ymandel added inline comments.
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
98

LLVM style is to limit use of anonymous namespaces to class declarations (which have no parallel to static).

100

Good question. I prefer this style because the array form carries the bounds information with it, unlike the raw pointer form: https://godbolt.org/z/jEP9Wasrh But, I don't know of any meaningful performance implications. FWIW, this is the recommended form for our internal codebase.

120

inlined the matcher too

181

I'm fine with the current length. And, a little worried that "const" has its own meaning, which is separate from this one.

218

capitalized, but prefer to keep it separate for readability. Also, generally good practice since some compliers don't allow these kinds of strings inline in macro calls.

246

Actually, the eval engine is more powerful than you'd guess -- 1 + 2 evaluates statically. :)

sgatev accepted this revision.Dec 15 2021, 10:31 AM
sgatev added inline comments.
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
146

Move this above to replace the other call to getASTContext() or use getASTContext() everywhere?

This revision is now accepted and ready to land.Dec 15 2021, 10:31 AM
ymandel updated this revision to Diff 394617.Dec 15 2021, 11:25 AM
ymandel marked 5 inline comments as done.

address review comments

This revision was landed with ongoing or failed builds.Dec 15 2021, 11:34 AM
This revision was automatically updated to reflect the committed changes.