[PATCH] D37355: Add CalledValuePropagation pass

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

I see this review has been hanging around a while, so i'll take a stab at reviewing it.

(Random note: I know you didn't do this, but getOrInitValueState is very verbose for a thing that everything is probably going to use by default.
IMHO, the API should be getOrInitValueState is renamed to getValueState, getValueState is renamed to getExistingValueState.
Similarly with the other calls.

Since there are no other users, this may be a good time to change it in a followup)

Generally looks very very good. I don't think i have any algorithmic complaints here at all.
If you agree with the templating vs void ptrs, i'm happy to go commit that patch.



================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:203
+  /// The generic sparse propagation solver typedefs lattice values to void
+  /// pointers. Since our lattice values can't fit in the space of a pointer,
+  /// we have to unique them in a std::set and let the generic solver index
----------------
Again, not a thing you have to address here, but is there a reason it's not just templated?

IE define an interface lattice vals must meet, and template it relying on that interface to exist.

This is what we do with GraphTraits, for example.

Using void pointers and having to go through these machinations seems .... ugly

(Edit: I have a patch to template it, here: https://reviews.llvm.org/differential/diff/117397/)


================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:206
+  /// this set.
+  std::set<CVPLatticeVal> &LatticeVals;
+
----------------
DenseSet?

(Do you need the ordering?)



================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:228
+    if (!F)
+      IndirectCalls.insert(CS.getInstruction());
+
----------------
Nitpick: IndirectCalls.insert(I)


https://reviews.llvm.org/D37355