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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Other items discussed offline:

- the instruction usage feels to generic with too few uses.  I suggested (but will not require) either making them computed properties of an intrinsic, or finding other uses.
- my comment about using a blacklist is clearly wrong.  You'd suggested using an attribute in Instrinsics.td.  I was fine with that, but it felt like possible overkill.



================
Comment at: llvm/include/llvm/Analysis/LoopAllocationInfo.h:91
+  // deallocations.
+  SmallVector<const CallInst *, 4> Allocations;
+  SmallVector<SmallVector<CallInst *, 16>, 4> Deallocations;
----------------
It really looks like you have a named tuple hidding here.

AllocationInfoEntry?


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:857
+        HoistedInstructions.push_back(&I);
+        Instruction *Pattern = LAI->toSink(cast<CallInst>(&I))[0];
+        for (auto ExitBlock : LAI->addDeallocations(cast<CallInst>(&I))) {
----------------
As debated offline,  naming here is problematic.
- addDeallocations -> locationsNeedingFrees? newAllocPlacement? 
- toSink -> freesForMalloc
(key piece: avoid action names)

A comment would be help as well.

Pull out the cast to CallInst



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:860
+          Instruction *Clone = Pattern->clone();
+          Clone->dropUnknownNonDebugMetadata();
+          ExitBlock->getInstList().insert(ExitBlock->getFirstInsertionPt(),
----------------
You might wish to either a) union the source locations, or b) restrict this to non -g 

See  applyMergedLocation on Instruction.


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

https://reviews.llvm.org/D60056