[llvm] r270892 - [MemCpyOpt] Don't perform callslot optimization across may-throw calls

[David Majnemer via llvm-commits]

On Thu, May 26, 2016 at 1:21 PM, Eli Friedman <eli.friedman at gmail.com>
wrote:

> On Thu, May 26, 2016 at 12:24 PM, David Majnemer via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: majnemer
>> Date: Thu May 26 14:24:24 2016
>> New Revision: 270892
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=270892&view=rev
>> Log:
>> [MemCpyOpt] Don't perform callslot optimization across may-throw calls
>>
>> An exception could prevent a store from occurring but MemCpyOpt's
>> callslot optimization would fire anyway, causing the store to occur.
>>
>> This fixes PR27849.
>>
>> Added:
>>     llvm/trunk/test/Transforms/MemCpyOpt/callslot_throw.ll
>> Modified:
>>     llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>>     llvm/trunk/test/Transforms/MemCpyOpt/loadstore-sret.ll
>>
>> Modified: llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp?rev=270892&r1=270891&r2=270892&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Scalar/MemCpyOptimizer.cpp Thu May 26
>> 14:24:24 2016
>> @@ -496,7 +496,7 @@ static unsigned findCommonAlignment(cons
>>
>>  // This method try to lift a store instruction before position P.
>>  // It will lift the store and its argument + that anything that
>> -// lay alias with these.
>> +// may alias with these.
>>  // The method returns true if it was successful.
>>  static bool moveUp(AliasAnalysis &AA, StoreInst *SI, Instruction *P) {
>>    // If the store alias this position, early bail out.
>> @@ -675,6 +675,8 @@ bool MemCpyOpt::processStore(StoreInst *
>>        if (C) {
>>          // Check that nothing touches the dest of the "copy" between
>>          // the call and the store.
>> +        Value *CpyDest = SI->getPointerOperand()->stripPointerCasts();
>> +        bool CpyDestIsLocal = isa<AllocaInst>(CpyDest);
>>          AliasAnalysis &AA =
>> getAnalysis<AAResultsWrapperPass>().getAAResults();
>>          MemoryLocation StoreLoc = MemoryLocation::get(SI);
>>          for (BasicBlock::iterator I = --SI->getIterator(), E =
>> C->getIterator();
>> @@ -683,6 +685,12 @@ bool MemCpyOpt::processStore(StoreInst *
>>              C = nullptr;
>>              break;
>>            }
>> +          // The store to dest may never happen if an exception can be
>> thrown
>> +          // between the load and the store.
>> +          if (I->mayThrow() && !CpyDestIsLocal) {
>> +            C = nullptr;
>> +            break;
>> +          }
>>          }
>>        }
>>
>> @@ -815,6 +823,10 @@ bool MemCpyOpt::performCallSlotOptzn(Ins
>>      if (destSize < srcSize)
>>        return false;
>>    } else if (Argument *A = dyn_cast<Argument>(cpyDest)) {
>> +    // The store to dest may never happen if the call can throw.
>> +    if (C->mayThrow())
>> +      return false;
>>
>
> I'm not sure mayThrow is sufficient; it doesn't cover longjmp.  Maybe not
> worth worrying about, though.
>

Ah, great point!  IMO, we should model longjmp as a "throw" as the
optimizer has to worry about it in the same ways.


>
>
> -Eli
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160526/e549bc53/attachment.html>