[PATCH] D37355: Add CalledValuePropagation pass

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

As Eli, I'd like to see other examples (I'm confident you can find some). In the meanwhile, a first review.



================
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"));
----------------
Is there a way we can avoid somewhat arbitrary cutoffs? They've been us a lot in the past.


================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:52
+/// function.
+static bool canTrackFunctionInterprocedurally(Function *F) {
+  return F->hasExactDefinition() && F->hasLocalLinkage() &&
----------------
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.


================
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() ||
----------------
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).


================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:108
+  bool operator<(const CVPLatticeVal &O) const {
+    return LV < O.LV || Functions < O.Functions;
+  }
----------------
is this deterministic?


================
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()); }
----------------
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).


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


================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:344
+  auto *Overdefined = &*LatticeVals.emplace(CVPLatticeVal::Overdefined).first;
+  auto *Untracked = &*LatticeVals.emplace(CVPLatticeVal::Untracked).first;
+
----------------
Maybe at some point we can get rid of this.


================
Comment at: lib/Transforms/IPO/CalledValuePropagation.cpp:382
+    return PreservedAnalyses::all();
+  return PreservedAnalyses::none();
+}
----------------
I expected this to preserve something? [probably the same set that IPSCCP preserves]


https://reviews.llvm.org/D37355