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

[Hal Finkel]

----- 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, July 27, 2015 4:35:00 AM
> Subject: Re: [PATCH] Flag to enable IEEE-754 friendly FP optimizations
> 
> Hello Hal,
> 
> On Sun, Jul 26, 2015 at 07:29:13AM -0700, Hal Finkel wrote:
> > I apologize for the exceedingly-long delay in getting to these.
> 
> Better later than never :).  Thanks for reviewing it.
> 
> > 0004-Skip-const...serve-FPEnv.patch:
> >
> > +  /// Evaluates floating point operation to examine it, does
> > nothing for non-FP
> > +  /// operations.  Returns false for unsupported values.
> > +  static bool getFPOpExceptions(const Value *V, APFloat::opStatus
> > &S);
> >
> > The comment on this function does not really explain what it does.
> > Is it getting exceptions that are definitely generated, or
> > potentially generated? The comment should say.
> 
> Definitely generated exceptions at run-time, that are lost if
> expression
> is folded at compile-time.
> 
> > 0005-Teach-IR-b...t-new-flags.patch:
> >
> > The IRBuilder parts of this need to change slightly. You can set
> > the fast-math flags that the builder sets on all relevant
> > instructions using
> > SetFastMathFlags/getFastMathFlags/clearFastMathFlags, and so you
> > don't need the extra parameters (we don't have them for any of the
> > other flags).
> >
> > Maybe the get* functions in Constants.h should just take a
> > FastMathFlags structure, instead of adding parameters for each
> > flag?
> 
> 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.

> 
> > 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.
> 
> > In the future, it is much easier to review these patches on
> > reviews.llvm.org, see for instructions:
> >
> >   http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
> 
> I'm familiar with Phabricator, but is it able to handle multiple
> patches
> well? 

No, you'd need to create separate 'differentials' for each patch.

> Unless I'm missing something, it can only show all them
> squashed (and
> I don't think it's good for review) or completely separated.  In the
> latter case if first patch is changed the rest twelve will require
> updates
> as well. 

Yes, this is somewhat unfortunate.

> What's the recommended way of publishing changes consisting
> of
> multiple patches via Phabricator?

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.

> 
> Will address comments and update patches soon.

Great, thanks!

 -Hal

> 
> Thanks,
> Sergey
> 

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