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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

I suspect at least one of your bugs is that the param state lattice doesn't get updated when the value state lattice changes.



================
Comment at: lib/Transforms/Scalar/SCCP.cpp:307
+  ValueLatticeElement getLatticeValueFor(Value *V) {
+    if (ParamState.count(V) == 0) {
+      DenseMap<Value *, LatticeVal>::const_iterator I = ValueState.find(V);
----------------
Use find so you aren't doing this lookup twice.
Or even better, please use insert to make space if it doesn't exist, it won't screw up your assert.



================
Comment at: lib/Transforms/Scalar/SCCP.cpp:311
+             "V not found in ValueState nor Paramstate map!");
+      ParamState[V] = I->second.toValueLattice();
+    }
----------------
This seems wrong.
What happens if the value state lattice value changes?
This will never update it.



================
Comment at: lib/Transforms/Scalar/SCCP.cpp:314
+
+    return ParamState[V];
   }
----------------
Then you can just return the result of the insert here.



================
Comment at: lib/Transforms/Scalar/SCCP.cpp:450
+
+    if (ParamState.count(V) == 0)
+      ParamState[V] = getValueState(V).toValueLattice();
----------------
Ditto above on how to do this without repeated lookups


================
Comment at: lib/Transforms/Scalar/SCCP.cpp:451
+    if (ParamState.count(V) == 0)
+      ParamState[V] = getValueState(V).toValueLattice();
+
----------------
Ditto above, if the value changes you will never update it from the value state.


================
Comment at: lib/Transforms/Scalar/SCCP.cpp:1600
+  // Currently we only use range information for integer values.
+  if (!(V->getType()->isIntegerTy() && IV.isConstantRange()))
+    return false;
----------------
You already assert it's an integer type above?
Choose a single place to do it ;)


https://reviews.llvm.org/D36656