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

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Mostly minor comments.


================
Comment at: include/llvm/Analysis/Loads.h:65
@@ +64,3 @@
+/// Scan the ScanBB block backwards checking to see if we have the value at
+/// the memory address *Ptr locally available within a small number of
+/// instructions. If the value is available, return it.
----------------
Nit here and later: \p Ptr

(This is optional) Also, instead of a separate line about `AccessTy`, I'd just restate this to say "if we have the value at address \p Ptr of type \p AccessTy"

================
Comment at: include/llvm/Analysis/Loads.h:80
@@ +79,3 @@
+/// is found, it is left unmodified.
+///
+/// This function returns value available at memory address *Ptr of type AccessTy.
----------------
Describe `isAtomicMemOp`, and note that we're constant folding an unordered load (actually, maybe that's a better name for this function - `FoldLoadFromAddress`?).

Also, by convention, it should be `IsAtomicMemOp`.

================
Comment at: include/llvm/Analysis/Loads.h:82
@@ +81,3 @@
+/// This function returns value available at memory address *Ptr of type AccessTy.
+/// All sanity checks to confirm if this function should be called and whether
+/// the value returned can be used should be done at the caller.
----------------
I think "sanity checks" is too vague to be useful.  I think it is sufficient to state that we've assumed that we're folding a non-volatile unordered (but potentially atomic) load -- it is understood that people calling this function know what they want. :)

================
Comment at: lib/Analysis/Loads.cpp:363
@@ +362,3 @@
+    if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
+      Value *LoadPtr = LI->getPointerOperand()->stripPointerCasts();
+      if (AreEquivalentAddressValues(LoadPtr, StrippedPtr) &&
----------------
This bit of change looks unnecessary now


http://reviews.llvm.org/D21246