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

[Nick Lewycky via Phabricator via llvm-commits]

nicholas marked 6 inline comments as done.
nicholas added inline comments.


================
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.
----------------
reames wrote:
> either "may ... ?" or "return true if ...".  (i.e make a question a question)
Done, used "return true iff".


================
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;
----------------
reames wrote:
> Should we assert that CI is a malloc within the specified loop?
We don't presently store the right member variables to assert that, and I don't really want to add DEBUG-only member variables. The condition on this function is that mayHoist(CI) must be true. I've added an assert that CI is in entries, which is the condition under which this function would have UB.


================
Comment at: llvm/lib/Analysis/LoopAllocationInfo.cpp:92
+       II != E; ++II) {
+    if (!II->mayDeallocateMemory() && !II->mayAllocateMemory())
+      continue;
----------------
reames wrote:
> As discussed offline, bug example:
> for (int i = 0; i < N; i++) {
>   throw_if(requested_size > TOO_BIG);
>   a = malloc(requested_size);
>   free(a);
> }
I've updated this to use isGuaranteedToTransferExecutionToSuccessor on the instructions leading up to the allocation. The "throw_if" example couldn't happen because any opaque function might also malloc or free, so the test @test16 uses a volatile load instead.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:497
 
+    bool PassedOpaqueFunction = LAI->empty();
     for (BasicBlock::iterator II = BB->end(); II != BB->begin();) {
----------------
Rename this to something clearer, such as "ScanForFrees".


================
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))) {
----------------
reames wrote:
> 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
> 
Is picking the first one out of toSink/freesForMalloc a problem? The order depends on the use-list ordering. I don't know what LLVM's current rules on that are, I know that there's https://llvm.org/docs/LangRef.html#use-list-order-directives . Is it considered a bug to have an optimization whose result depends on use list order?


================
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))) {
----------------
nicholas wrote:
> reames wrote:
> > 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
> > 
> Is picking the first one out of toSink/freesForMalloc a problem? The order depends on the use-list ordering. I don't know what LLVM's current rules on that are, I know that there's https://llvm.org/docs/LangRef.html#use-list-order-directives . Is it considered a bug to have an optimization whose result depends on use list order?
Is it possible for a free call to have an operand bundle on a malloc or free call? In CloneInstructionInExitBlock, LICM updates the funclet operand bundle for the new location in the CFG.


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

This loop skips the terminator, so it can't be an invoke.

Which is, indeed, a miscompile bug. Fixed above where we initialize ScanForFrees and added @test19.

Changed it to CallBase here anyways.


================
Comment at: llvm/test/Transforms/LICM/allocs.ll:567
+; CHECK: @free
+  %Val = load volatile i8, i8* %vptr
+  %ptr = call i8* @malloc(i32 %loop_invariant)
----------------
reames wrote:
> 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.
This test shows the difference from using isGuaranteedToTranferExecution in analyzeLoop. Before that change, we would have hoisted the malloc above the volatile load. The same test written with a function would not have shown any difference simply because there is no way to create a function that might-throw but can't-malloc/free (without adding new attributes to LLVM). It would also just be @test3 again.

I think the new behaviour tested for here is correct. Suppose you have a system where out of memory terminates the program, and the volatile store is triggering some external action (moves the robot arm). Suppose the programmer is using volatile to make sure the external system is in the safe state before doing the malloc that may terminate, and wants the malloc to complete before beginning any more operations. The compiler should not reorder these actions.


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

https://reviews.llvm.org/D60056