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

[Sanjoy Das]

REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:228
@@ +227,3 @@
+           "can not insert explicit cast");
+    // hasOneUse property is guaranted by "EstimateCostForTruncatedEvaluation"
+    assert(I->hasOneUse() &&
----------------
Spelling: guaranteed

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:238
@@ +237,3 @@
+    Instruction *NextI = I->getNextNode();
+    assert(NextI != I->getParent()->end() &&
+           "can not insert explicit truncate");
----------------
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)
```

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:340
@@ +339,3 @@
+/// specified expression tree as type Ty instead of its larger type, and
+/// arrive with the same value. Also calculate cost of this transformation
+/// and store it into the "Cost" argument.
----------------
Nit: "arrive at the same value"

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

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:343
@@ +342,3 @@
+/// Currently cost model is simple - increase cast by one for each additional
+/// instruction required, decrese cost by one for each instruction that
+/// will be removed after truncating expression tree.
----------------
Nit: decrease

================
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
----------------
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)

```


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:453
@@ -427,3 +452,3 @@
     // TODO: Can handle more cases here.
     break;
   }
----------------
I'd either remove the `default: break;` or move this bit of logic into the `default:` clause.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:463
@@ +462,3 @@
+/// IsProfitableToEvaluateTruncated - Return true if it would be profitable to
+/// evaluate the specified expression tree as type Ty instead of its larger
+/// type, and arrive with the same value. This is used by code that tries to
----------------
What is `Ty`?

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:469
@@ +468,3 @@
+///
+static bool IsProfitableToEvaluateTruncated(Instruction *V,
+                                            InstCombiner &IC) {
----------------
Use either `Instruction *I` or `Value *V`.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:352
@@ -337,3 +351,3 @@
 
   Instruction *I = dyn_cast<Instruction>(V);
   if (!I) return false;
----------------
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?

http://reviews.llvm.org/D10571

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