Adds another constant-propagation analysis that covers all variables in
the scope (vs the existing single-variable demo). But, the analysis is still
unsuited to use, in that ignores issues of escaping variables.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp | ||
---|---|---|
375–376 | This seems incorrect. The lattice elements for target and other should be top at p2. |
Fix handling of uninitialized variables.
clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp | ||
---|---|---|
375–376 | Yes, great point. I've updated the tests and fixed handling of uninitialized variables. |
Does it make sense to have both the single and multi variable tracking tests? Or do we want to have these as some sort of steps in a tutorial?
Right -- the intent in keeping the original one was for an (eventual) tutorial. For that matter, if you think there's changes I should make to the single variable version to bring them closer together, I'd be happy to do that.
clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp | ||
---|---|---|
10 | Indent two spaces. | |
161 | Consider removing this return and moving the assignment that follows in an else block. I think this way the two cases (with and without initializer) will be more apparent. | |
163 | I believe this should be bottom as ValueLattice::bottom models an undefined value. | |
172–174 | I think this makes the transfer function non-monotonic. Concretely, Vars[var] can be top and the right-hand side can be something else. The transfer function must be monotonic to guarantee termination. Perhaps this should be Vars[Var].join(...)? |
clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp | ||
---|---|---|
163 | No, because it's not truly undefined -- it has either a default value or implementation-defined value (for scalars). So, an uninitialized variable has some value, just an unknown one. | |
172–174 | Keep in mind that CP analysis is not looking at a variable's value *over the course of program*, but only *at each program point*. If we wanted to know whether a variable could be replaced entirely with a constant, then we'd need the former, in which case you'd be right that join would be correct here. To the more general point: monotonic means something more subtle. Specifically, it doesn't relate inputs to outputs, but input-output pairs to each other. Specifically, for monotinically increasing functions f: if a <= b then f(a) <= f(b) In this case, no matter the inputs to f, the output is a constant (the value of the expression at that point or top), which trivially satisfies the requirement f(a) <= f(b). Intuitively, transfer functions model program behavior, so indeed it would be odd to enforce that x <= f(x). Assignment is a good example -- at that program point, we know *exactly* the value of the variable, no matter what it held before. |
I think from the tutorial's point of view that would be awesome! But I'm also happy with doing that in a follow-up PR some later time.
clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp | ||
---|---|---|
54 | If ValueSate only makes sense when Value is none, this almost looks like a std::variant<ValueState, int64_t>. Unfortunately, as far as I understand LLVM is not at C++17 yet. I did not find an equivalent structure in the LLVM support library, but in case you agree, I think we could leave a TODO to change this after the C++ standard requirement was raised. |
clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp | ||
---|---|---|
172–174 | Agreed about the distinction in the type of analysis. What I meant is that ConstantPropagationAnalysis::transfer isn't monotonic because it could happen that transfer(top, ...) = 1 and transfer(1, ...) = top. However, I think this isn't important. What's important is that transferBlock is monotonic which I think is the case for ConstantPropagationAnalysis. |
clang/unittests/Analysis/FlowSensitive/MultiVarConstantPropagationTest.cpp | ||
---|---|---|
105 | Hello! I commit a tiny fix here, please see: rGa5af260d3e8b00a3353e813b73f83391edbef493 |
Indent two spaces.