[PATCH] D37198: [InlineCost] add visitSelectInst()

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

Just wanted to say thanks so much for working on this -- its a huge improvement to the inline cost.



================
Comment at: lib/Analysis/InlineCost.cpp:1201
+    // Select True, X, Y => X
+    if (CondC->isAllOnesValue() && TrueC) {
+      SimplifiedValues[&SI] = TrueC;
----------------
No need to separately test for true and false once you know the condition is a constant. Just check `isNullValue()`. It's either false, or true. =]


================
Comment at: lib/Analysis/InlineCost.cpp:1212-1217
+    if (TrueC && FalseC) {
+      if (Constant *C = ConstantExpr::getSelect(CondC, TrueC, FalseC)) {
+        SimplifiedValues[&SI] = C;
+        return true;
+      }
+    }
----------------
I feel like we should handle this above when handling `TrueC == FalseC`.


================
Comment at: lib/Analysis/InlineCost.cpp:1222-1249
+  if (SI.getType()->isPointerTy()) {
+    std::pair<Value *, APInt> TrueBaseAndOffset =
+        ConstantOffsetPtrs.lookup(TrueVal);
+    bool TrueSROACandidate = false;
+    Value *TrueSROAArg;
+    if (TrueBaseAndOffset.first) {
+      DenseMap<Value *, int>::iterator CostIt;
----------------
I think this code will be substantially simpler if you refactor it in the following way.

First, check if the condition is a constant. If *not* a constant, then handle the only cases we can handle in that world: equal (or constant expr-able) inputs, or matching SROA args like here. Then return.

Rest of the function only handling when it *is* a constant. Then assign the input the constant will select to a variable, and only do all the checks on that one variable rather on both true and false variants.


================
Comment at: lib/Analysis/InlineCost.cpp:496
+                                       bool AllConstants) {
+  SmallVector<Value *, 2> Ops;
   for (Value *Op : I.operands()) {
----------------
eraman wrote:
> Sorry, I should have thought through this carefully.  My bad. I feel this version is worse than before. Most callers of simplifyInstruction pass a callable that takes a vector of Value * and implicitly treats it as a vector of Constant *.  Feel free to go back the the previous version or come up with a better way to refactor simplifyInstruction.  My main motivation for the refactoring was to keep accesses to SimplifiedValues in one location, so that we can get data on how often does inlining actually simplifies a value to a constant. 
FWIW, I agree. I looked at both versions, and I think the custom select handling is better. It's hard to tell ahead of time, so still, thanks for exploring whether this helps.


Repository:
  rL LLVM

https://reviews.llvm.org/D37198