[llvm] r270892 - [MemCpyOpt] Don't perform callslot optimization across may-throw calls
David Majnemer via llvm-commits
llvm-commits at lists.llvm.org
Thu May 26 15:19:40 PDT 2016
On Thu, May 26, 2016 at 3:00 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>
> ------------------------------
>
> *From: *"David Majnemer" <david.majnemer at gmail.com>
> *To: *"Hal Finkel" <hfinkel at anl.gov>
> *Cc: *"llvm-commits" <llvm-commits at lists.llvm.org>, "Eli Friedman" <
> eli.friedman at gmail.com>
> *Sent: *Thursday, May 26, 2016 4:52:48 PM
>
> *Subject: *Re: [llvm] r270892 - [MemCpyOpt] Don't perform callslot
> optimization across may-throw calls
>
>
>
> 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.
>
>
> Yes (although for local variables, I think they need to be volatile).
>
The wording in the standard is a little more specific than that. It says
that any modification that happens between the setjmp and longjmp can only
be observed using volatile. After the longjmp, has jumped back to the
setjmp you are not obligated to keep using volatile.
C11 7.13.2.1:
All accessible objects have values, and all other components of the
abstract machine have state, as of the time the longjmp function was
called, except that the values of objects of automatic storage duration
that are local to the function containing the invocation of the
corresponding setjmp macro that do not have volatile-qualified type and
have been changed between the setjmp invocation and longjmp call are
indeterminate.
In the above "program", 'y' could be set to a non-zero value if we have
never performed a 'longjmp'. We then 'longjmp' back to the 'setjmp', store
zero into 'y' and dutifully skip over the 'longjmp' on our second way
through. We would then execute the store of 9 to 'x' which, by my reading
of the spec, should not require any volatile annotation whatsoever.
> More broadly, however, we don't want to mark all functions that might call
> longjmp as mayThrow(), because that would essentially be all external
> functions?
>
It depends. How well do we want to model the semantics of setjmp/longjmp?
FWIW there is precedence for using unwinding to model this in LLVM, we
currently model pthread cancelation semantics via unwind (e.g. open may
throw...)
I think that if we as a community really care about setjmp/longjmp, we
should model them better. If we feel like our current model is sufficient,
then that's fine too.
Personally, I have no dog in this fight.
>
>
> -Hal
>
>
>
>> -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
>>
>
>
>
>
> --
> 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/bbc4c7fc/attachment.html>
More information about the llvm-commits
mailing list