[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:238
@@ +237,3 @@
+    Instruction *NextI = I->getNextNode();
+    assert(NextI != I->getParent()->end() &&
+           "can not insert explicit truncate");
----------------
sanjoy wrote:
> I still don't understand this assert:  why can't you have IR like:
> 
> ```
> %i = add i32 ...  ;; I == %i
> br label %foo
> 
> foo:
> use(%i)
> ```
I->getParent()->end() is end of the instruction list, not a terminator instruction. We will fail this assertion if "I" was a terminator. In other words "I++ == BB->end()"

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:360
@@ -345,2 +359,3 @@
   if ((isa<ZExtInst>(I) || isa<SExtInst>(I)) &&
-      I->getOperand(0)->getType() == Ty)
+      I->getOperand(0)->getType() == Ty) {
+    // Add a cost bonus because we can eliminate one additional instruction
----------------
sanjoy wrote:
> We can eliminate the extension only if it has at most one use.  Right now, if I'm reading the code correctly, you'll consider this case to be profitable:
> 
> 
> ```
> X = f()
> Y = g()
> 
> A = zext(B) from i32 to i64
> C = zext(D) from i32 to i64
> 
> M = A + C + X + Y
> 
> use(A)
> use(C)
> 
> ```
> 
> and translate it to
> 
> ```
> X = f()
> Y = g()
> 
> A = zext(B) from i32 to i64
> C = zext(D) from i32 to i64
> 
> M = B + D + trunc(X) + trunc(Y)
> 
> use(A)
> use(C)
> 
> ```
> 
That's right. I fixed the patch to update cost only for casts with single use.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:352
@@ -337,3 +351,3 @@
 
   Instruction *I = dyn_cast<Instruction>(V);
   if (!I) return false;
----------------
sanjoy wrote:
> igor-laevsky wrote:
> > 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.
> > We can handle Argument by adding special case into "EvaluateInDifferentType"
> 
> Why would you need any special casing in `EvaluateInDifferentType`?  Doesn't it have a catch-all that will take any `iN` to `iM` by using a `trunc` instruction?
Yes, but special case is required to determine place where to insert this additional explicit trunc. I am not saying this is anyhow complicated, just seems more like a separate change.

http://reviews.llvm.org/D10571

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