[PATCH] D37355: Add CalledValuePropagation pass

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added a comment.

In https://reviews.llvm.org/D37355#886149, @dberlin wrote:

> 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.


Thanks for the review! Yes, templating would be much nicer than void pointers. Feel free to go ahead with that, and I'll update this patch accordingly.



================
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
----------------
dberlin wrote:
> 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/)
Yeah, that would be much nicer - the generic solver is old code. Thanks for working on the patch, and please feel free to go ahead and commit it. You should also update the comment at the top of AbstractLatticeFunction, since it talks about the void pointer. I can rebase my work on top of that.


================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:206
+  /// this set.
+  std::set<CVPLatticeVal> &LatticeVals;
+
----------------
dberlin wrote:
> DenseSet?
> 
> (Do you need the ordering?)
> 
Sounds good. If we template the lattice function, I don't think std::set will be needed anymore.

Because the values had to be void pointer's, I needed addresses of my custom values in order to interface with the generic solver. DenseSet couldn't be used because the values can move around in memory after they've been added to the set.


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


https://reviews.llvm.org/D37355