[PATCH] D36656: [SCCP] Propagate integer range info for parameters in IPSCCP.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for having another look and for your comments. I'll address them soon. I am not sure about ValueState and ParamState entries diverging (I responded with details inline), but I am probably missing something.



================
Comment at: lib/Transforms/Scalar/SCCP.cpp:311
+             "V not found in ValueState nor Paramstate map!");
+      ParamState[V] = I->second.toValueLattice();
+    }
----------------
dberlin wrote:
> This seems wrong.
> What happens if the value state lattice value changes?
> This will never update it.
> 
My understanding was that this function only gets called after the solver finished, meaning the value in ValueState should not change.


================
Comment at: lib/Transforms/Scalar/SCCP.cpp:451
+    if (ParamState.count(V) == 0)
+      ParamState[V] = getValueState(V).toValueLattice();
+
----------------
dberlin wrote:
> Ditto above, if the value changes you will never update it from the value state.
Hm, this function is only used to get the state for function parameters. I thought the only place where we change the state for function parameters is at line 1194 and there is no other place where  the ValueState entries of function parameters could be changed. At least that was what I intended :)


https://reviews.llvm.org/D36656