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

[Igor Laevsky]

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");
+
----------------
sanjoy wrote:
> Don't construct a `std::string` here -- just using `I->getName() + ".truncated"` will construct an `llvm::Twine` for you.
done

================
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() &&
----------------
sanjoy wrote:
> Use `InsertBefore = I->getNextNode()`
done

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

================
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
----------------
sanjoy wrote:
> Nit: "increase"
done

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

We can handle Argument by adding special case into "EvaluateInDifferentType".

Probably for MetadataAsValue and BasicBlock we can just assert false - should not meet this instructions in arithmetic expression trees.

InlineAsm we can probably handle similarly to the Argument, but I am not absolutely sure how it works.

This are possible extensions, but they feel more like a separate change.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:469
@@ +468,3 @@
+///
+static bool IsProfitableToEvaluateTruncated(Instruction *V,
+                                            InstCombiner &IC) {
----------------
sanjoy wrote:
> I'd just move this within `InstCombiner`.  Then you won't need `IC` and will be able to use `ShouldChangeType` without making it `public`.
Agree. Actually both "IsProfitableToEvaluateTruncated" and "EstimateCostForTruncatedEvaluation" should live inside InstCombiner. I don't want to add too much unrelated refactoring, so I will do this in a separate change and move "ShouldChangeType" back to the private section.

http://reviews.llvm.org/D10571

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