[PATCH] Allow InstCombiner to eliminate truncates even if it will require inserting additional instructions

[Sanjoy Das]

Overall, I think this is going in the right direction.

Some comments inline.


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:234
@@ +233,3 @@
+    Instruction *Trunc = CastInst::CreateTruncOrBitCast(I, Ty);
+    Trunc->setName(std::string(I->getName()) + ".truncated");
+
----------------
Don't construct a `std::string` here -- just using `I->getName() + ".truncated"` will construct an `llvm::Twine` for you.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:237
@@ +236,3 @@
+    // Insert new truncate after "I"
+    Instruction *InsertBefore = std::next(BasicBlock::iterator(I));
+    assert(InsertBefore != I->getParent()->end() &&
----------------
Use `InsertBefore = I->getNextNode()`

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:239
@@ +238,3 @@
+    assert(InsertBefore != I->getParent()->end() &&
+           "can not visit terminator instructions");
+    return InsertNewInstBefore(Trunc, *InsertBefore);
----------------
I'm not sure I understand this assert -- `InsertBefore` is not the instruction instcombine visited, right?

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:342
@@ +341,3 @@
+/// and store it into the "Cost" argument.
+/// Currently cost model is simple - increse cast by one for each additional
+/// instruction required, decrese cost by one for each instruction that
----------------
Nit: "increase"

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:345
@@ +344,3 @@
+/// will be removed after truncating expression tree.
+static bool EstimateCostForTruncatedEvaluation(int &Cost, Value *V,
+                                               Type *Ty, InstCombiner &IC,
----------------
If we change the bit handing the `!isa<Instruction>` case below, will we need this to return a `bool`?  Or will it always "succeed"?

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:352
@@ -337,3 +351,3 @@
 
   Instruction *I = dyn_cast<Instruction>(V);
   if (!I) return false;
----------------
Why does the `This is not a recognized instruction` bit apply here?

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:469
@@ +468,3 @@
+///
+static bool IsProfitableToEvaluateTruncated(Instruction *V,
+                                            InstCombiner &IC) {
----------------
I'd just move this within `InstCombiner`.  Then you won't need `IC` and will be able to use `ShouldChangeType` without making it `public`.

http://reviews.llvm.org/D10571

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/