[PATCH] D37355: Add CalledValuePropagation pass

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added a comment.

Hi Davide,

Thanks very much for the initial review! I'm working on adding a few more motivating test cases that demonstrate missed optimization opportunities. I'll upload those shortly.



================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:36-38
+static cl::opt<unsigned> MaxFunctionsPerValue(
+    "cvp-max-functions-per-value", cl::Hidden, cl::init(4),
+    cl::desc("The maximum number of functions to track per lattice value"));
----------------
davide wrote:
> Is there a way we can avoid somewhat arbitrary cutoffs? They've been us a lot in the past.
The cut-off used here is to prevent the number of lattice values we have to maintain in a `std::set` from growing too large. As I mentioned in the comments, the number of possible values could technically grow quite large (set of all subsets). I don't think this is likely to occur in practice, though.

One thing we could do to relax any cut-off is to make the lattice values take up less space. We could do this by representing the set of target functions as a bit vector, for example, instead of a vector of function pointers. For this particular task, though, I'm not sure how useful it would be to leave the number of target functions unconstrained.

I envisioned two main uses for this work (although I'm sure we can come up with more): indirect call promotion, and intersecting function attributes at call sites. My thinking was that if we wanted to do indirect call promotion, we probably would want to give up if the call could target a lot of functions (i.e., if we end up having to do more than insert a simple if-else). Intersecting attributes is less clear to me, but I was thinking that the chance that all possible call targets would have interesting attributes (say, a `norecurse` or `readnone`) would be small if the set of targets gets very large.


================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:52
+/// function.
+static bool canTrackFunctionInterprocedurally(Function *F) {
+  return F->hasExactDefinition() && F->hasLocalLinkage() &&
----------------
davide wrote:
> As there's an increasing interest in IPO in llvm (yay) we might consider taking this kind of generic function and move to a common helper (e.g. IPO/Utils). FWIW, GCC has something like that. Also, I'm pretty sure IPSCCP has the same exact function (at least as far as I remember) and in case we find a bug there we need to update a single copy.
This makes sense to me. Yes, the check for trackable functions and global variables is more-or-less taken from IPSCCP.


================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:58
+/// Determine if we can track the values stored in the given global variable.
+static bool canTrackGlobalVariableInterprocedurally(GlobalVariable *GV) {
+  if (GV->isConstant() || !GV->hasLocalLinkage() ||
----------------
davide wrote:
> I'm unsure about this one. I had a bug lying around as sparse conditional constant propagation has a similar problem (i.e. it ignores variables which address is taken, although I had hard time to understand the concept of address taken for GVs).
I agree. This was PR33143, for reference. I had originally thought we could add a `hasAddressTaken` to `GlobalVariable` similar to what we have in `Function`, but after hearing feedback from Eli, I'm not sure that would help. In this case and in IPSCCP, we're interested in whether we can track the values loaded/stored at a given global variable. This is why we also have to check that the memory operations are not volatile.


================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:108
+  bool operator<(const CVPLatticeVal &O) const {
+    return LV < O.LV || Functions < O.Functions;
+  }
----------------
davide wrote:
> is this deterministic?
It is, but it doesn't look that way because I sort and unique the functions vector before constructing the lattice values (in `MergeValues`). The functions vector is essentially a sorted set that I tried to make more efficient by using a SmallVector. Also, the functions vector doesn't change after a lattice value object is constructed. Multiple LLVM values will map to the same lattice value object (see also the DenseSet comment below).


================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:141
+  /// this. It's likely better to not waste space tracking irrelevant values.
+  /// @{
+  bool IsUntrackedValue(Value *V) { return !isPointerToFunction(V->getType()); }
----------------
davide wrote:
> I'm not familiar with this style of comments, and I don't think it's actually common in LLVM (although I may be wrong).
It's used in a few places, but I'm happy to document these functions separately. My aim was to not be overly repetitive.


================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:338
+  // set.
+  std::set<CVPLatticeVal> LatticeVals;
+
----------------
davide wrote:
> DenseSet?
As written, I don't think `DenseSet` will work, unfortunately. Internally, the generic solver represents lattice values as void pointers (it's old code I guess), so they either have to fit in that amount of space (like the `PointerIntPair` in SCCP) or they have to index some uniquing data structure. `std::set` doesn't invalidate iterators after insertion like `DenseSet` does. That let's us use the address of our custom lattice values as the void pointer in the generic solver. This means that `LatticeVals` holds the unique lattice value objects, and multiple LLVM values end up mapping to the address of same lattice value object. It's possible there is a better way to interface with the generic solver, though. 


================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:344
+  auto *Overdefined = &*LatticeVals.emplace(CVPLatticeVal::Overdefined).first;
+  auto *Untracked = &*LatticeVals.emplace(CVPLatticeVal::Untracked).first;
+
----------------
davide wrote:
> Maybe at some point we can get rid of this.
I agree. Eli mentioned this as well. Untracked and overdefined are more-or-less the same.


================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:382
+    return PreservedAnalyses::all();
+  return PreservedAnalyses::none();
+}
----------------
davide wrote:
> I expected this to preserve something? [probably the same set that IPSCCP preserves]
This is what IPSCCP does, actually. But in this case, since we're only adding metadata we could just preserve all. We could even make this an analysis, like TBAA, which also just attaches metadata.


https://reviews.llvm.org/D37355