[PATCH] D21791: [InstCombine] Fix for trunc folding build break

[Anna Thomas via llvm-commits]

anna added inline comments.

================
Comment at: lib/Analysis/Loads.cpp:328
@@ +327,3 @@
+/// caller.
+Value *llvm::FindAvailableLoadedValue(Value *Ptr, Type *AccessTy,
+                                      bool IsAtomicMemOp, BasicBlock *ScanBB,
----------------
reames wrote:
> I'm not following why you need this new interface.  Why can't we reuse the existing interface?  We have the load instruction don't we?
> 
> ... I think I see it.  The issue is we need to use the narrower type right?  The comment seams to be stale.  You're really using the size of the access ty.  Can you adjust the comment to make this more obvious.
> 
> p.s. Please don't repeat function comments in the cpp.  And yes, I know the near by code does.  This is older style.
This function `FindAvailableLoadedValue` is used to see if we have the contents of type `AccessTy` at location `Ptr`. 

This function can be called for either full or partial sizes, as mentioned in start of the comment:
```Scan the ScanBB block backwards checking to see if we have the value at the memory address \p Ptr of type \p AccessTy```

Will remove the comment from cpp, and try to make the need for the interface more obvious.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:595
@@ +594,3 @@
+      if (Value *AvailableVal = FindAvailableLoadedValue(
+              LI->getPointerOperand(), DestTy, LI->isAtomic(), LI->getParent(),
+              BBI, DefMaxInstsToScan))
----------------
reames wrote:
> I think what you've got here is entirely correct, but let me sketch and alternate framing to see if you think this is simpler.  Instead of splitting out the properties of the load, we leave the interface that of the load, but add the accessty (which is asserts as less than the load type).  If we do that, the atomicity check can be sunk into the common code.  
We had this in one of the iterations in prior review. 

We changed the function signature to be a memory location, rather than an load instruction (http://reviews.llvm.org/D21246#463566). 

Discussed with Sanjoy and reasons were:
1. we can use this function to see (partial or full) memory contents of any operation -- memcpy, load etc. Required constraints should be verified at the caller, as mentioned in function comments.
```
/// Note that we assume the \p *Ptr is accessed through a non-volatile but
/// potentially atomic load. Any other constraints should be verified at the
/// caller.
```
2. Use `FindAvailableLoadedValue` as-is in most optimizations which require the full load value. No change to function signature. 

I had added the Type *accessTy to the original interface with the LoadInst, but this was error prone since there was ambiguity on whether LoadInst->getType() or AccessTy (from visitTrunc) are used. Your suggestion would allow us to use a single type `accessTy` , but change the calls to this function used in various optimizations. 


Repository:
  rL LLVM

http://reviews.llvm.org/D21791