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

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: include/llvm/Analysis/Loads.h:64
@@ -63,1 +63,3 @@
 
+/// FindAvailableMemoryContents - Return the value if the partial or full memory
+/// contents of the Load are available. The partial or full type is specified
----------------
Nit: newer code does not repeat the function name before comments.

================
Comment at: include/llvm/Analysis/Loads.h:67
@@ +66,3 @@
+/// through AccessTy. The sanity checks necessary for any transformation done
+/// based on the  available memory value should be done by the callers of this
+/// function.
----------------
This is not quite the interface I was thinking of.  I was thinking of

```
Value *FindAvailableMemoryContents(Value *Location, Type *AccessTy,
                                   BasicBlock *BB,
                                   BasicBlock::iterator &ScanFrom,
                                   unsigned MaxInstsToScan,
                                   AliasAnalysis *AA = nullptr,
                                   AAMDNodes *AATags = nullptr,
                                   bool *IsLoadCSE = nullptr);

```

That is, `FindAvailableMemoryContents` tries to give a `Value *` that would be the result of loading a value of type `AccessTy` from `Location` (this is how it should be specified too).  Whether the `Location` came from a load instruction of elsewhere is not something it needs to worry about.

================
Comment at: lib/Analysis/Loads.cpp:303
@@ -302,25 +302,3 @@
 
-/// \brief Scan the ScanBB block backwards to see if we have the value at the
-/// memory address *Ptr locally available within a small number of instructions.
-///
-/// The scan starts from \c ScanFrom. \c MaxInstsToScan specifies the maximum
-/// instructions to scan in the block. If it is set to \c 0, it will scan the whole
-/// block.
-///
-/// If the value is available, this function returns it. If not, it returns the
-/// iterator for the last validated instruction that the value would be live
-/// through. If we scanned the entire block and didn't find something that
-/// invalidates \c *Ptr or provides it, \c ScanFrom is left at the last
-/// instruction processed and this returns null.
-///
-/// You can also optionally specify an alias analysis implementation, which
-/// makes this more precise.
-///
-/// If \c AATags is non-null and a load or store is found, the AA tags from the
-/// load or store are recorded there. If there are no AA tags or if no access is
-/// found, it is left unmodified.
-Value *llvm::FindAvailableLoadedValue(LoadInst *Load, BasicBlock *ScanBB,
-                                      BasicBlock::iterator &ScanFrom,
-                                      unsigned MaxInstsToScan,
-                                      AliasAnalysis *AA, AAMDNodes *AATags,
-                                      bool *IsLoadCSE) {
+/// \brief Return the value if the partial or full memory
+/// contents of the Load is available. The partial or full type is specified
----------------
Nit: remove the `\brief` -- newer code can rely on doxygen using autobrief.

================
Comment at: lib/Analysis/Loads.cpp:313
@@ -312,3 @@
-/// through. If we scanned the entire block and didn't find something that
-/// invalidates \c *Ptr or provides it, \c ScanFrom is left at the last
-/// instruction processed and this returns null.
----------------
Why did you remove this?  Isn't this pertinent to `FindAvailableMemoryContents` as well?

================
Comment at: lib/Analysis/Loads.cpp:455
@@ +454,3 @@
+
+  // All the logic resides in FindAvailableMemoryContents which returns partial
+  // or complete load value depending on AccessTy. FindAvailableLoadedValue is a
----------------
I think this "partial or complete" term (here and elsewhere) are more confusing than helpful.  The invariant here is that `FindAvailableMemoryContents` will return a value of type `AccessTy`, whether `AccessTy` is partial or full from the perspective of the caller is not something `FindAvailableMemoryContents` should worry about.  For instance, with the edited interface that takes a pointer and not a `LoadInst`, I'd rather we have:

```
// We want a value covering the whole of the load.
return FindAvailableMemoryContents(Load->getPointerOperand(), Load->getType(), ScanBB, ScanFrom, MaxInstsToScan, AA, AATags, IsLoadCSE);
```



Repository:
  rL LLVM

http://reviews.llvm.org/D21246