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

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: lib/Analysis/Loads.cpp:385
@@ +384,3 @@
+      // If we support partial value forwarding to the load (PartialType is
+      // non-null): LI is from a part of the bits (known from PartialType)
+      // used by Load, return the value for those bits. The PartialType is from
----------------
anna wrote:
> sanjoy wrote:
> > It feels odd that we have to special-case `PartialType` this way.  I think a better design would be to introduce a new function `FindAvailableMemoryContents(Value *Ptr, Type *AccessTy ... < no PartialTy>)` (perhaps with a different name), and have this version of `FindAvailableLoadedValue` dispatch to `FindAvailableMemoryContents` one with `LI->getPointerOperand(), LI->getType()`.  `visitTrunc` can then call `FindAvailableMemoryContents` directly.
> It seems ok, but are we fine with changing `FindAvailableLoadedValue` to being a wrapper function, with all the actual code in `FindAvailableMemoryContents`?
I don't see any problems with that (you can put them both in this .cpp file for the time being).

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:588
@@ +587,3 @@
+    if (Value *AvailableVal = FindAvailableLoadedValue(
+            LI, LI->getParent(), BBI, DefMaxInstsToScan,
+            /* AA */ nullptr, /* AATags */ nullptr, DestTy))
----------------
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
}
```



http://reviews.llvm.org/D21246