[PATCH] D30104: Refactor instruction simplification code in visitors. NFC.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: lib/Analysis/InlineCost.cpp:211
   bool visitUnreachableInst(UnreachableInst &I);
+  /// Simplify \p I if its operands are constants and update SimplifiedValues.
+  /// Evaluate is a lambda specific to instruction type that evaluates the
----------------
I'd keep this above with the general implementation routines rather than with the visitors.

Also, I'd keep the comment on the implementation rather than the declaration.


================
Comment at: lib/Analysis/InlineCost.cpp:216
+  simplifyInstruction(Instruction &I,
+                      function_ref<Constant *(ArrayRef<Constant *>)> Evaluate);
 
----------------
Using a function_ref seems like a fairly heavy abstraction. Maybe make this a template and take an arbitrary callable?


================
Comment at: lib/Analysis/InlineCost.cpp:465
+  int N = 0;
+  for (Value *Op : I.operands()) {
+    Constant *COp = dyn_cast<Constant>(Op);
----------------
What if there are more than two operands? Maybe you want to require one of th eexpression subclasses here? Or maybe just make COps be a small vector so this is generically correct? It should't be any less efficient, and I find the N++ bit below somewhat easy to miss anyways.


================
Comment at: lib/Analysis/InlineCost.cpp:473-476
+  if (auto *C = Evaluate(COps)) {
+    SimplifiedValues[&I] = C;
+    return true;
+  }
----------------
I would invert and use early return here as well:

  auto *C = Evaluate(COpts);
  if (!C)
    return false;

  SimplifiedValues[&I] = C;
  return true;


================
Comment at: lib/Analysis/InlineCost.cpp:507-509
+  auto Evaluate = [&](ArrayRef<Constant *> COps) -> Constant * {
+    return ConstantExpr::getPtrToInt(COps[0], I.getType());
+  };
----------------
For all of these where you only use the lambda once, put the lambda in the arguments?

Also for all of these, can you let the return type be deduced? I always prefer that when it is unambiguous. This might be made easier by the routine accepting a generic callable.


https://reviews.llvm.org/D30104