[PATCH] Flag to enable IEEE-754 friendly FP optimizations

[Hal Finkel via llvm-commits]

----- Original Message -----
> From: "Sergey Dmitrouk" <sdmitrouk at accesssoftek.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Owen Anderson" <owen at apple.com>, "llvm-commits" <llvm-commits at cs.uiuc.edu>
> Sent: Monday, August 17, 2015 12:19:58 PM
> Subject: Re: [PATCH] Flag to enable IEEE-754 friendly FP optimizations
> 
> Hi Hal,
> 
> On Mon, Aug 03, 2015 at 10:19:13PM -0700, Hal Finkel wrote:
> > > Will address comments and update patches soon.
> >
> > Great, thanks!
> 
> Sorry, didn't have enough time to complete this before vacation.  The
> reason
> it required more time is that unfortunate instruction reordering when
> floating-point operation is moved below function invocation.  You
> mentioned
> it in the winter, but I couldn't reproduce the issue no matter how
> hard I tried
> and thought that it can't happen.  After rebase three weeks ago it
> occurred for the first time allowing me to make a test for it, but
> defining
> operation to read/write from memory didn't fix the issue.  I left it
> on
> trying to find more checks that should block undesired reordering
> during
> instruction selection, will get back to it.
> 
> > > If I remember correctly, FastMathFlags is not used in those
> > > sources and I
> > > didn't want to introduce such dependency, but if that's OK, I'd
> > > prefer to have FastMathFlags there.
> >
> > Please do that.
> 
> It turns out I already tried, but forget why it didn't work: it
> introduces circular dependency between IR/Constants.h and
> IR/Operator.h
> (FastMathFlags is defined here and this header includes Constants.h).

Just move them into their own header if necessary.

Thanks again,
Hal

> 
> > > > 0013-Don-t-hois...cts-in-LICM.patch:
> > > >
> > > > I don't understand why we're using a custom check here. It
> > > > should
> > > > just check if the operation is safe to speculatively execute,
> > > > no?
> > >
> > > Don't remember, but will check that.
> 
> +  if (const FPMathOperator *MathOp =
> dyn_cast<FPMathOperator>(&Inst))
> +    if (MathOp->hasKeepExceptions() || MathOp->hasKeepRounding())
> +      return false;
> +
>    if (isSafeToSpeculativelyExecute(&Inst, CtxI, DT, TLI))
>      return true;
> 
> isSafeToSpeculativelyExecute() is not enough here, because its result
> is
> taken into account only when it's positive and we need to forbid the
> case
> where result is negative.  I'll add a comment and change order of the
> checks.
> 
> > No, you'd need to create separate 'differentials' for each patch.
> > I'm not sure that there is one. In the past, we've just opened
> > multiple
> > differentials, sometimes tagging them as '1 of N' in the title for
> > clarity.
> 
> OK, thanks, will do that for next series of patches.
> 
> Regards,
> Sergey
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory