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

[David Majnemer via llvm-commits]

On Thu, May 26, 2016 at 1:57 PM, Hal Finkel <hfinkel at anl.gov> wrote:

>
>
> ------------------------------
>
> *From: *"David Majnemer via llvm-commits" <llvm-commits at lists.llvm.org>
> *To: *"Eli Friedman" <eli.friedman at gmail.com>
> *Cc: *"llvm-commits" <llvm-commits at lists.llvm.org>
> *Sent: *Thursday, May 26, 2016 3:33:13 PM
> *Subject: *Re: [llvm] r270892 - [MemCpyOpt] Don't perform callslot
> optimization across may-throw calls
>
>
>
>
> 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.
>
> Don't we model it with some kind of does-not-return annotation?
>

That is likely insufficient without changing what noreturn implies.

Calling a noreturn function implies that simple memory operations before
the call to allocas are not observable.
For example:

int x;
int y;
...
if (y)
  exit(0);
x = 9;

Here, we are free to hoist that store to 'x' above the call to 'exit' so
long as it doesn't escape (because of 'atexit' handlers).

The following is a 'setjmp'/'longjmp' version of the above:

int x;
int y;
...
if (y)
  longjmp(jb, 1);
x = 9;

Here it would not be safe to hoist the store to 'x'.  This is because the
'longjmp' might take us to a place where 'x' is still live.



>  -Hal
>
>
>
>>
>>
>> -Eli
>>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160526/ca01e646/attachment.html>