[PATCH] D60056: Hoist/sink malloc/free's in LICM.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

minor comments on interface, LICM part, and tests.  I got interrupted and need to come back and finish reviewing the analysis implementation.



================
Comment at: llvm/include/llvm/Analysis/LoopAllocationInfo.h:77
+
+  // Given an allocation call, are all the safety conditions met to hoist this
+  // allocation and sink its deallocations.
----------------
either "may ... ?" or "return true if ...".  (i.e make a question a question)


================
Comment at: llvm/include/llvm/Analysis/LoopAllocationInfo.h:82
+  // Retrieve the list of free calls in the loop matching a given allocation.
+  const SmallVector<CallInst *, 16> &FreesForMalloc(CallInst *CI) const {
+    return Entries.find(CI)->second.Deallocations;
----------------
Should we assert that CI is a malloc within the specified loop?


================
Comment at: llvm/include/llvm/Analysis/LoopAllocationInfo.h:106
+    // Known deallocations of this pointer. These are guaranteed to deallocate
+    // the pointer if executed. (Excludes "free(expr ? ptr1 : nullptr);".)
+    SmallVector<CallInst *, 16> Deallocations;
----------------
Not sure what the last part of the comment means.  Reword?


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:535
+        } else if (isa<CallInst>(I) &&
+                   (!isa<IntrinsicInst>(I) ||
+                    match(&I, PatternMatch::m_Intrinsic<
----------------
What about invokes?  (Use callbase)


================
Comment at: llvm/test/Transforms/LICM/allocs.ll:567
+; CHECK: @free
+  %Val = load volatile i8, i8* %vptr
+  %ptr = call i8* @malloc(i32 %loop_invariant)
----------------
This test is odd.  I don't see anything preventing us from reordering the volatile load and malloc.  Was this an attempt to test for a side exit?  If so, simply using an unknown call before the malloc would be preferred.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60056/new/

https://reviews.llvm.org/D60056