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

[Hal Finkel via llvm-commits]

----- Original Message -----

> 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). 

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? 

-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/9d27cc37/attachment.html>