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

[Philip Reames via llvm-commits]

reames accepted this revision.
reames added a comment.

LGTM w/comments addressed.


================
Comment at: lib/Analysis/Loads.cpp:328
@@ +327,3 @@
+/// caller.
+Value *llvm::FindAvailableLoadedValue(Value *Ptr, Type *AccessTy,
+                                      bool IsAtomicMemOp, BasicBlock *ScanBB,
----------------
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.

================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:595
@@ +594,3 @@
+      if (Value *AvailableVal = FindAvailableLoadedValue(
+              LI->getPointerOperand(), DestTy, LI->isAtomic(), LI->getParent(),
+              BBI, DefMaxInstsToScan))
----------------
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.  

================
Comment at: test/Transforms/InstCombine/trunc.ll:186
@@ +185,3 @@
+
+declare void @consume(i8) readonly
+define i1 @trunc_load_store(i8* align 2 %a) {
----------------
Add a comment about this only be legal for non-atomic loads since you essentially split the load here.

================
Comment at: test/Transforms/InstCombine/trunc.ll:198
@@ +197,3 @@
+; CHECK: call void @consume(i8 0)
+}
+
----------------
Add check lines for the other half of the original load.  

================
Comment at: test/Transforms/InstCombine/trunc.ll:213
@@ +212,3 @@
+; CHECK-NOT: trunc
+}
+
----------------
Add positive checks for the two consume lines at least.

================
Comment at: test/Transforms/InstCombine/trunc.ll:258
@@ +257,3 @@
+; CHECK-NOT: %wide.load = load atomic i16, i16* %bca unordered, align 2
+}
+
----------------
Add a positive test for the two consume lines.

================
Comment at: test/Transforms/InstCombine/trunc.ll:276
@@ +275,3 @@
+; trunc cannot be replaced since store size is not trunc result size
+define i1 @trunc_different_size_load(i16 * align 2 %a) {
+  store i16 0, i16 *%a, align 2
----------------
This looks to be a missed optimization.  Please comment it as such.


Repository:
  rL LLVM

http://reviews.llvm.org/D21791