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

Mostly minor suggestions below...



================
Comment at: lib/Analysis/InlineCost.cpp:1186-1190
+  Constant *TrueC = isa<Constant>(TrueVal) ? cast<Constant>(TrueVal)
+                                           : SimplifiedValues.lookup(TrueVal);
+  Constant *FalseC = isa<Constant>(FalseVal)
+                         ? cast<Constant>(FalseVal)
+                         : SimplifiedValues.lookup(FalseVal);
----------------
I'd still use dyn_cast here:

  Constant *TrueC = dyn_cast<Constant>(TrueVal);
  if (!TrueC)
    TrueC = SimplifiedValues.lookup(TrueVal);



================
Comment at: lib/Analysis/InlineCost.cpp:1196
+    // Select C, X, X => X
+    if ((TrueC || FalseC) && TrueC == FalseC) {
+      SimplifiedValues[&SI] = TrueC;
----------------
I'd simplify this as: `TrueC == FalseC && TrueC`.


================
Comment at: lib/Analysis/InlineCost.cpp:1230
+    if (TrueC && FalseC) {
+      if (Constant *C = ConstantExpr::getSelect(CondC, TrueC, FalseC)) {
+        SimplifiedValues[&SI] = C;
----------------
auto?


================
Comment at: lib/Analysis/InlineCost.cpp:1238-1239
+
+  // Condition is either all 1s or all 0s. SI can be simplified.
+  if (Constant *SelectedC = CondC->isAllOnesValue() ? TrueC : FalseC) {
+    SimplifiedValues[&SI] = SelectedC;
----------------
I think just `dyn_cast<Constant>(SelectedV)` is easier to read and no more expensive.


Repository:
  rL LLVM

https://reviews.llvm.org/D37198