[PATCH] D21246: [InstCombine] Add rule to fold away trunc of partial load

[Anna Thomas via llvm-commits]

anna added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:588
@@ +587,3 @@
+    if (Value *AvailableVal = FindAvailableLoadedValue(
+            LI, LI->getParent(), BBI, DefMaxInstsToScan,
+            /* AA */ nullptr, /* AATags */ nullptr, DestTy))
----------------
sanjoy wrote:
> anna wrote:
> > sanjoy wrote:
> > > Here if we have:
> > > 
> > > ```
> > > store i64 %x to %ptr
> > > %val = load i64* %ptr
> > > %val.tr = trunc i64 %val to i32
> > > ```
> > > 
> > > won't we have a type mismatch and try to replace `%val.tr` with `%x`?
> > We won't replace `%val.tr` with `%x` since the `DestTy` is `i32`. We do the check in  `FindAvailableLoadedValue (LI: %val,.., DestTy: i32)` that `i64` is `BitorNoopPointerCastable` to the PartialType `i32`. 
> > I've added this test case as well now. 
> I was thinking of the case where the first check, i.e.
> 
> ```
>       if (AreEquivalentAddressValues(LoadPtr, StrippedPtr) &&
>           CastInst::isBitOrNoopPointerCastable(LI->getType(), AccessTy, DL)) {
> ```
> 
> or
> 
> ```
>       if (AreEquivalentAddressValues(StorePtr, StrippedPtr) &&
>           CastInst::isBitOrNoopPointerCastable(SI->getValueOperand()->getType(),
>                                                AccessTy, DL)) {
> ```
> 
> would fire and you'd return a wider value.  So the test case has to be
> 
> ```
> define i1 @trunc_different_size_load(i32 * align 4 %a) {
>   store i32 0, i32 *%a, align 4
>   %wide.load = load i32, i32* %a, align 4
>   %lowhalf = trunc i32 %wide.load to i8
>   call void @consume(i8 %lowhalf)
>   %cmp.2 = icmp ult i32 %wide.load, 256
>   ret i1 %cmp.2
> ; CHECK-LABEL: @trunc_different_size_load
> ; CHECK: %lowhalf = trunc i32 %wide.load to i8
> }
> ```
> 
I see your point about the widening and the mismatch of types. The reason the firing of the first check did not trigger any failures was because a prior instcombine rule (I think it is combineLoadStore which calls `FindAvailableLoad` as well) already removes the trunc. So with or without this trunc rule in instcombine, opt with -instcombine changes the above test case to:
```
define i1 @trunc_different_size_loads(i32* align 4 %a) {
  store i32 0, i32* %a, align 4
  call void @consume(i8 0)
  ret i1 false
}
```
Also verified the value is correctly truncated, say in this case:
```
  store i32 257, i32* %a, align 4
  call void @consume(i8 1)
  ret i1 false
```

That said, I agree we should not be relying on a certain ordering of instcombine rules for this, since there would be a type mismatch in absence of load store instcombine rules.
Will make the change. Thanks :)


http://reviews.llvm.org/D21246