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

[Sergey Dmitrouk]

Hi Hal,

On Tue, Nov 11, 2014 at 10:29:11AM -0800, Hal Finkel wrote:
> Hi Sergey,
>
> 0001-Add-HonorFPExceptions-flag-to-TargetOptions.patch:
>
> +    /// HonorFPExceptions - This flag is enabled when code relies on
> +    /// availability of floating point exceptions at run-time, thus such
> +    /// operations should not be replaced with their results at compile-time.
> +    unsigned HonorFPExceptions : 1;
>
> These patches seem to fall under the general category of allowing FP environment access. As a result, I think that naming this AllowFPEnvAccess is better, and indicates that the code is allowed to inspect (via exceptions or otherwise) the state of the FP environment, and thus we need to avoid constant folding that affects the FP environment, and also avoid reordering that does the same.

Yes, "AllowFPEnvAccess" would be a better name, I'll rename it.

> I'm not sure there is value is separating out the exceptions and/or constant-folding case from the rest. On the other hand, allowing access to the FP exception state should perhaps be different from allowing the changing of rounding modes?

I'm not sure we can actually separate them, both seems to depend on
folding (not sure which rounding is used at compile time, but it
probably some fixed rounding mode) and ordering (rounding mode is set via
function call).  One flag should be enough, at least I don't see cases
when separate flags would affect output differently.

> Also, there seem to be two possible designs here:
>   1. A function-level attribute (which is what you have now)
>   2. An instruction-level flag (meaning generalizing the current "fast math flags" to also have a "keep exceptions" flag).
>
> I think that, unless there is actually some reason it is not workable, option (2) seems better -- it allows for more fine-grained control, interacts better with LTO, and elides the whole issue of passing this global flag around.
>
> If, however, we go with option (1) [which is what you currently have], then obviously the naming issue above applies to all of the patches, but aside from that...

I chose function-level attributes as I didn't notice fast math flags
being there (I was looking for instruction attributes instead, which
don't exist).  I agree that option (2) is better and will update patches
accordingly, thanks.

> 0002-Consider-HonorFPExceptions-flag-in-SelectionDAG.patch:
>
> +  /// Checks whether it's safe to replace floating point operation with it's
>
> replace floating point operation -> replace a floating-point operation

And "with it's" -> "with its".

> Also, requires a test, otherwise LGTM
>
> 0003-Skip-constant-folding-to-preserve-FP-exceptions.patch:
>
> LGTM
>
> 0004-Pass-HonorFPExceptions-flag-around.patch:
>
> LGTM
>
> 0005-Use-honor-fp-exceptions-function-attribute.patch:
>
> +  // Target cares about IEEE754 conformant FP exceptions at run-time?
> +  HonorFPExceptions = F.hasFnAttribute("honor-fp-exceptions") &&
> +                      (F.getFnAttribute("honor-fp-exceptions")
> +                        .getValueAsString() == "true");
>
> This should be a function somewhere.
>
> Also, the InstCombine change requires a test.
>
> 0006-Add-honor-fp-exceptions-option-to-LLVM.patch:
>
> LGTM
>
> 0007-Do-not-fold-constants-on-reading-in-IR-assembly.patch:
>
> Requires a test, otherwise LGTM
>
> 0008-Consider-HonorFPExceptions-flag-in-SimplifyCFG.patch:
>
> LGTM (assuming use of the new attribute function as noted above)
>
> 0009-Add-LLVM-test-for-HonorFPExceptions.patch:
>
> LGTM (this test should go with the associated patch)

Could only merge it into the previous patch as if at least one change is
missing the test fails.  Maybe it won't be this way when I add separate
tests for patches as you request.

Thank you for the review, will work on updated version.

Best regards,
Sergey

> Thanks again,
> Hal
>
> ----- Original Message -----
> > From: "Sergey Dmitrouk" <sdmitrouk at accesssoftek.com>
> > To: "llvm-commits" <llvm-commits at cs.uiuc.edu>
> > Cc: "Hal Finkel" <hfinkel at anl.gov>, "Stephen Canon" <scanon at apple.com>, "Owen Anderson" <owen at apple.com>
> > Sent: Friday, October 31, 2014 11:19:40 AM
> > Subject: Re: [PATCH] Flag to enable IEEE-754 friendly FP optimizations
> >
> > Ping.  Rebased patches with a fix for a typo (in code, not in
> > comments) are
> > attached.
> >
> > Hal, do I get it right that what you suggest can be implemented in
> > Instruction::mayReadFromMemory() and Instruction::mayWriteToMemory()
> > to
> > get desired behaviour in wider range of usages?
> >
> > Thanks,
> > Sergey
> >
> > On Fri, Oct 24, 2014 at 09:24:48AM -0700, Stephen Canon wrote:
> > >      On Oct 24, 2014, at 12:23 PM, Hal Finkel <hfinkel at anl.gov>
> > >      wrote:
> > >
> > >      ----- Original Message -----
> > >
> > >        From: "Stephen Canon" <scanon at apple.com>
> > >        To: "Hal Finkel" <hfinkel at anl.gov>
> > >        Cc: "Sergey Dmitrouk" <sdmitrouk at accesssoftek.com>,
> > >        "llvm-commits"
> > >        <llvm-commits at cs.uiuc.edu>, "Owen Anderson"
> > >        <owen at apple.com>
> > >        Sent: Friday, October 24, 2014 11:13:10 AM
> > >        Subject: Re: [PATCH] Flag to enable IEEE-754 friendly FP
> > >        optimizations
> > >
> > >        As a necessary piece of building support for FENV_ACCESS,
> > >        yes, I
> > >        think this is worth pursuing.  Note, however, that actually
> > >        providing full FENV_ACCESS support is likely to be a
> > >        significant
> > >        undertaking, and I expect that the pieces that go into it
> > >        are
> > >        basically useless until all of them are in place.  Thata**s
> > >        not to say
> > >        that it isna**t worth doing, just that ita**s a big
> > >        thankless job.
> > >
> > >      Steve, so long as you're willing to serve as the expert
> > >      reviewer here,
> > >      I'm happy to help move this forward as well. We should,
> > >      however,
> > >      probably have a plan for the rest of it. Maybe this is as
> > >      simple as:
> > >      when FP exceptions are enabled, we treat all FP operations as
> > >      if they
> > >      might write to memory (except that AA confirms that they don't
> > >      actually
> > >      alias with any address in particular, but we say nothing about
> > >      function
> > >      calls). What do you think?
> > >
> > >    From a numerical point of view, something along those lines
> > >    could be
> > >    workable, yes.
> > >    a** Steve
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory