llvm dev - Nov 2016 - RFC: Consider changing the semantics of 'fast' flag implying all fast-math-flags

I don’t really like the idea of updating checks of UnsafeAlgebra() to depend on
all of the other flags.  It seems like it would be preferable to look at each
optimization and figure out which flags it actually requires.  I suspect that in
many cases the “new” flag (i.e. allowing reassociation, etc.) will be what is
actually needed anyway.

I would be inclined to agree with Niolai’s suggestion of combining all the flags
related to value safety, except that Warren’s proposal that started this
discussion seems to imply that he has a use case that requires reciprocals to be
turned off separately.

-Andy

From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of Mehdi
Amini via llvm-dev
Sent: Wednesday, November 16, 2016 8:55 AM
To: Ristow, Warren <warren.ristow at sony.com>
Cc: llvm-dev at lists.llvm.org
Subject: Re: [llvm-dev] RFC: Consider changing the semantics of 'fast'
flag implying all fast-math-flags


On Nov 15, 2016, at 11:59 PM, Ristow, Warren <warren.ristow at
sony.com<mailto:warren.ristow at sony.com>> wrote:

Hi,

Thanks for the quick feedback.  I see your points, but I have a few
questions/comments.  I'll start at the end of the previous post:
> ...
> I think these are valuable problems to solve, but you should tackle them
piece by piece:
>
> 1) the clang part of overriding the individual FMF and emitting the right
IR is the first thing to fix.
> 2) the backend is still using the global UnsafeFPMath and it should be
killed.
I addressed this point (2) for the reciprocal aspect in the patch, but of course
that wasn't useful without doing something about (1).

Regarding (1), over at https://reviews.llvm.org/D26708#596610, David made the
same point that it should be done in Clang.  I can understand that, but I wonder
whether having the concept of the 'fast' flag in the IR that implies all
the other FMF makes sense?  I'm not seeing a good reason for it, but since
this is very new to me, I can easily imagine I'm missing the big picture.

For example, in the LLVM IR (http://llvm.org/docs/LangRef.html#fast-math-flags)
the fast-math flags 'nnan', 'ninf', 'nsz',
'arcp' and 'fast’ are defined.  Except for 'fast', each of
these has a fairly specific definition of what they mean.  For example, for
'arcp':

    arcp => "Allow optimizations to use the reciprocal of an argument
rather
             than perform division."

'fast' is unusual, in that it describes a fairly generic set of
aggressive floating-point optimizations:

    fast => "Allow algebraically equivalent transformations that may
dramatically
            change results in floating point (e.g. reassociate). This flag
implies
            all the others."

Very loosely, 'fast' means "all the aggressive FP-transformations
that are not controlled by one of the other 4, plus it implies all the other
4".  If for terminology, we call those additional aggressive optimizations
'aggr', then we have:

    'fast' == 'aggr' + 'nnan' + 'ninf' +
'nsz' + 'arcp'

So as I see it, if we want to disable only one of the other ones (like
'arcp', in my case), there isn't any way to express that with these
IR flags defined this way.  In short, we cannot turn on all the flags besides
'arcp'.  To do that, what we want is that somehow for the Clang
switches:

  '-ffast-math -fno-reciprocal-math'

to ultimately result in LLVM IR that has the following flags on in appropriate
FP ops:

  'aggr' + 'nnan' + 'ninf' + ‘nsz'

Make sense, I missed that we can’t *subtract* from fast at the IR level.

I wouldn’t be opposed to have something along the line of “aggr”, but there is a
tradeoff: some transformation may be harder to guard with this model.

Maybe that could be a starting point: changing the “UnsafeAlgebra” bit in the
FMF to be “aggr” you mention and replace all the query to
FastMathFlags::UnsafeAlgebra() to return true if all the bits are set in the
Flags. This alone should be nothing more than a mechanical change I believe.
The important part is then auditing all the users of UnsafeAlgebra() in the
middle end and check if they can be “downgraded” to aggr safely: i.e. if they
don’t need aggr *and* another flag.

—
Mehdi








But I don't see a way to express 'aggr' in the IR.  We could do
this, if we change the definition of the IR 'fast' flag to remove that
sentence about implying all the others:

    fast => "Allow algebraically equivalent transformations that may
dramatically
            change results in floating point (e.g. reassociate).

(If we do something like that, we may want to change the name from
'fast' to something else (like 'aggr'), to avoid tying it too
closely to the concept of the '-ffast-math' switch.)

As an aside, I don't know if the "reassociate" example is the only
other transformation that's allowed by 'fast' (I presume it
isn't), but I think reassociation would be better expressed by a separate
flag, which could then be controlled independently via
'-f[no]-associative-math' switch.  Not having that flag exist separately
in the FMF is the origin of PR27372.  But creating that flag and using it in the
appropriate places would still run into these problems of 'fast'
implying all the others, which would make it impossible to disable reassociation
while leaving all the other FMF transformations enabled.

To ask a concrete question using the current definition of 'fast' (which
includes enabling reassociation, as the LLVM IR documentation of FMF says), how
can we express in the IR that reciprocal-transformations are not allowed, but
reassociation is allowed?

So the bottom line is that I do see there are issues in Clang that are relevant.
But as long as 'fast' means "'aggr' plus all the other FMF
transformations", I don't see how we can effectively disable a subset
of those other FMF transformations (while leaving 'aggr'
transformations, such as reassociation, enabled).  With that in mind, my patch
took one step in having 'fast' no longer imply all the others.

Thanks,
-Warren

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161117/540b2669/attachment.html>

> On Nov 16, 2016, at 4:13 PM, Kaylor, Andrew <andrew.kaylor at
intel.com> wrote:
> 
> I don’t really like the idea of updating checks of UnsafeAlgebra() to
depend on all of the other flags.
In my mind that’s mainly a way to avoid a “Big Bang” effect by staging the
changes, allowing a much “NFC” changes as possible, then updating individual
user of  UnsafeAlgebra() to use the right set of flags, and killing this API
when possible..

— 
Mehdi


> It seems like it would be preferable to look at each optimization and
figure out which flags it actually requires.  I suspect that in many cases the
“new” flag (i.e. allowing reassociation, etc.) will be what is actually needed
anyway.
>  
> I would be inclined to agree with Niolai’s suggestion of combining all the
flags related to value safety, except that Warren’s proposal that started this
discussion seems to imply that he has a use case that requires reciprocals to be
turned off separately.
>  
> -Andy
>   <>
>  <>From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On
Behalf Of Mehdi Amini via llvm-dev
> Sent: Wednesday, November 16, 2016 8:55 AM
> To: Ristow, Warren <warren.ristow at sony.com>
> Cc: llvm-dev at lists.llvm.org
> Subject: Re: [llvm-dev] RFC: Consider changing the semantics of
'fast' flag implying all fast-math-flags
>  
>  
> On Nov 15, 2016, at 11:59 PM, Ristow, Warren <warren.ristow at sony.com
<mailto:warren.ristow at sony.com>> wrote:
>  
> Hi,
>  
> Thanks for the quick feedback.  I see your points, but I have a few
questions/comments.  I'll start at the end of the previous post:
>  
> > ...
> > I think these are valuable problems to solve, but you should tackle
them piece by piece:
> > 
> > 1) the clang part of overriding the individual FMF and emitting the
right IR is the first thing to fix.
> > 2) the backend is still using the global UnsafeFPMath and it should be
killed.
>  
> I addressed this point (2) for the reciprocal aspect in the patch, but of
course that wasn't useful without doing something about (1).
>  
> Regarding (1), over at https://reviews.llvm.org/D26708#596610
<https://reviews.llvm.org/D26708#596610>, David made the same point that
it should be done in Clang.  I can understand that, but I wonder whether having
the concept of the 'fast' flag in the IR that implies all the other FMF
makes sense?  I'm not seeing a good reason for it, but since this is very
new to me, I can easily imagine I'm missing the big picture.
>  
> For example, in the LLVM IR
(http://llvm.org/docs/LangRef.html#fast-math-flags
<http://llvm.org/docs/LangRef.html#fast-math-flags>) the fast-math flags
'nnan', 'ninf', 'nsz', 'arcp' and 'fast’ are
defined.  Except for 'fast', each of these has a fairly specific
definition of what they mean.  For example, for 'arcp':
>  
>     arcp => "Allow optimizations to use the reciprocal of an
argument rather
>              than perform division."
>  
> 'fast' is unusual, in that it describes a fairly generic set of
aggressive floating-point optimizations:
>  
>     fast => "Allow algebraically equivalent transformations that
may dramatically
>             change results in floating point (e.g. reassociate). This flag
implies
>             all the others."
>  
> Very loosely, 'fast' means "all the aggressive
FP-transformations that are not controlled by one of the other 4, plus it
implies all the other 4".  If for terminology, we call those additional
aggressive optimizations 'aggr', then we have:
>  
>     'fast' == 'aggr' + 'nnan' + 'ninf' +
'nsz' + 'arcp'
>  
> So as I see it, if we want to disable only one of the other ones (like
'arcp', in my case), there isn't any way to express that with these
IR flags defined this way.  In short, we cannot turn on all the flags besides
'arcp'.  To do that, what we want is that somehow for the Clang
switches:
>  
>   '-ffast-math -fno-reciprocal-math'
>  
> to ultimately result in LLVM IR that has the following flags on in
appropriate FP ops:
>  
>   'aggr' + 'nnan' + 'ninf' + ‘nsz'
>  
> Make sense, I missed that we can’t *subtract* from fast at the IR level.
>  
> I wouldn’t be opposed to have something along the line of “aggr”, but there
is a tradeoff: some transformation may be harder to guard with this model.
>  
> Maybe that could be a starting point: changing the “UnsafeAlgebra” bit in
the FMF to be “aggr” you mention and replace all the query to
FastMathFlags::UnsafeAlgebra() to return true if all the bits are set in the
Flags. This alone should be nothing more than a mechanical change I believe.
> The important part is then auditing all the users of UnsafeAlgebra() in the
middle end and check if they can be “downgraded” to aggr safely: i.e. if they
don’t need aggr *and* another flag.
>  
> — 
> Mehdi
>  
>  
>  
>  
>  
> 
> 
>  
> But I don't see a way to express 'aggr' in the IR.  We could do
this, if we change the definition of the IR 'fast' flag to remove that
sentence about implying all the others:
>  
>     fast => "Allow algebraically equivalent transformations that
may dramatically
>             change results in floating point (e.g. reassociate).
>  
> (If we do something like that, we may want to change the name from
'fast' to something else (like 'aggr'), to avoid tying it too
closely to the concept of the '-ffast-math' switch.)
>  
> As an aside, I don't know if the "reassociate" example is the
only other transformation that's allowed by 'fast' (I presume it
isn't), but I think reassociation would be better expressed by a separate
flag, which could then be controlled independently via
'-f[no]-associative-math' switch.  Not having that flag exist separately
in the FMF is the origin of PR27372.  But creating that flag and using it in the
appropriate places would still run into these problems of 'fast'
implying all the others, which would make it impossible to disable reassociation
while leaving all the other FMF transformations enabled.
>  
> To ask a concrete question using the current definition of 'fast'
(which includes enabling reassociation, as the LLVM IR documentation of FMF
says), how can we express in the IR that reciprocal-transformations are not
allowed, but reassociation is allowed?
>  
> So the bottom line is that I do see there are issues in Clang that are
relevant.  But as long as 'fast' means "'aggr' plus all the
other FMF transformations", I don't see how we can effectively disable
a subset of those other FMF transformations (while leaving 'aggr'
transformations, such as reassociation, enabled).  With that in mind, my patch
took one step in having 'fast' no longer imply all the others.
>  
> Thanks,
> -Warren
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161116/54ad4497/attachment.html>

>In my mind that’s mainly a way to avoid a “Big Bang” effect by staging the
changes, allowing a much
>“NFC” changes as possible, then updating individual user of  UnsafeAlgebra()
to use the right set of
>flags, and killing this API when possible..
Yeah, I thought that was your motivation.  Maybe I misunderstood your comment
about auditing current users of UnsafeAlgebra().  I initially read it as
implying that there might be some oddity in the current implementation that some
code is relying on that we’d need to look for.  Since the flag is distinct, the
fact that it “implies” the others seems like a potential hazard so a problem
like that wouldn’t surprise me.

If you were just suggesting that the changes to individual callers can be
deferred, then I can live with that.

-Andy

From: mehdi.amini at apple.com [mailto:mehdi.amini at apple.com]
Sent: Wednesday, November 16, 2016 4:20 PM
To: Kaylor, Andrew <andrew.kaylor at intel.com>
Cc: Ristow, Warren <warren.ristow at sony.com>; llvm-dev at
lists.llvm.org; Nicolai Hähnle <nhaehnle at gmail.com>
Subject: Re: [llvm-dev] RFC: Consider changing the semantics of 'fast'
flag implying all fast-math-flags


On Nov 16, 2016, at 4:13 PM, Kaylor, Andrew <andrew.kaylor at
intel.com<mailto:andrew.kaylor at intel.com>> wrote:

I don’t really like the idea of updating checks of UnsafeAlgebra() to depend on
all of the other flags.

In my mind that’s mainly a way to avoid a “Big Bang” effect by staging the
changes, allowing a much “NFC” changes as possible, then updating individual
user of  UnsafeAlgebra() to use the right set of flags, and killing this API
when possible..

—
Mehdi




It seems like it would be preferable to look at each optimization and figure out
which flags it actually requires.  I suspect that in many cases the “new” flag
(i.e. allowing reassociation, etc.) will be what is actually needed anyway.

I would be inclined to agree with Niolai’s suggestion of combining all the flags
related to value safety, except that Warren’s proposal that started this
discussion seems to imply that he has a use case that requires reciprocals to be
turned off separately.

-Andy

From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of Mehdi
Amini via llvm-dev
Sent: Wednesday, November 16, 2016 8:55 AM
To: Ristow, Warren <warren.ristow at sony.com<mailto:warren.ristow at
sony.com>>
Cc: llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
Subject: Re: [llvm-dev] RFC: Consider changing the semantics of 'fast'
flag implying all fast-math-flags


On Nov 15, 2016, at 11:59 PM, Ristow, Warren <warren.ristow at
sony.com<mailto:warren.ristow at sony.com>> wrote:

Hi,

Thanks for the quick feedback.  I see your points, but I have a few
questions/comments.  I'll start at the end of the previous post:
> ...
> I think these are valuable problems to solve, but you should tackle them
piece by piece:
>
> 1) the clang part of overriding the individual FMF and emitting the right
IR is the first thing to fix.
> 2) the backend is still using the global UnsafeFPMath and it should be
killed.
I addressed this point (2) for the reciprocal aspect in the patch, but of course
that wasn't useful without doing something about (1).

Regarding (1), over at https://reviews.llvm.org/D26708#596610, David made the
same point that it should be done in Clang.  I can understand that, but I wonder
whether having the concept of the 'fast' flag in the IR that implies all
the other FMF makes sense?  I'm not seeing a good reason for it, but since
this is very new to me, I can easily imagine I'm missing the big picture.

For example, in the LLVM IR (http://llvm.org/docs/LangRef.html#fast-math-flags)
the fast-math flags 'nnan', 'ninf', 'nsz',
'arcp' and 'fast’ are defined.  Except for 'fast', each of
these has a fairly specific definition of what they mean.  For example, for
'arcp':

    arcp => "Allow optimizations to use the reciprocal of an argument
rather
             than perform division."

'fast' is unusual, in that it describes a fairly generic set of
aggressive floating-point optimizations:

    fast => "Allow algebraically equivalent transformations that may
dramatically
            change results in floating point (e.g. reassociate). This flag
implies
            all the others."

Very loosely, 'fast' means "all the aggressive FP-transformations
that are not controlled by one of the other 4, plus it implies all the other
4".  If for terminology, we call those additional aggressive optimizations
'aggr', then we have:

    'fast' == 'aggr' + 'nnan' + 'ninf' +
'nsz' + 'arcp'

So as I see it, if we want to disable only one of the other ones (like
'arcp', in my case), there isn't any way to express that with these
IR flags defined this way.  In short, we cannot turn on all the flags besides
'arcp'.  To do that, what we want is that somehow for the Clang
switches:

  '-ffast-math -fno-reciprocal-math'

to ultimately result in LLVM IR that has the following flags on in appropriate
FP ops:

  'aggr' + 'nnan' + 'ninf' + ‘nsz'

Make sense, I missed that we can’t *subtract* from fast at the IR level.

I wouldn’t be opposed to have something along the line of “aggr”, but there is a
tradeoff: some transformation may be harder to guard with this model.

Maybe that could be a starting point: changing the “UnsafeAlgebra” bit in the
FMF to be “aggr” you mention and replace all the query to
FastMathFlags::UnsafeAlgebra() to return true if all the bits are set in the
Flags. This alone should be nothing more than a mechanical change I believe.
The important part is then auditing all the users of UnsafeAlgebra() in the
middle end and check if they can be “downgraded” to aggr safely: i.e. if they
don’t need aggr *and* another flag.

—
Mehdi









But I don't see a way to express 'aggr' in the IR.  We could do
this, if we change the definition of the IR 'fast' flag to remove that
sentence about implying all the others:

    fast => "Allow algebraically equivalent transformations that may
dramatically
            change results in floating point (e.g. reassociate).

(If we do something like that, we may want to change the name from
'fast' to something else (like 'aggr'), to avoid tying it too
closely to the concept of the '-ffast-math' switch.)

As an aside, I don't know if the "reassociate" example is the only
other transformation that's allowed by 'fast' (I presume it
isn't), but I think reassociation would be better expressed by a separate
flag, which could then be controlled independently via
'-f[no]-associative-math' switch.  Not having that flag exist separately
in the FMF is the origin of PR27372.  But creating that flag and using it in the
appropriate places would still run into these problems of 'fast'
implying all the others, which would make it impossible to disable reassociation
while leaving all the other FMF transformations enabled.

To ask a concrete question using the current definition of 'fast' (which
includes enabling reassociation, as the LLVM IR documentation of FMF says), how
can we express in the IR that reciprocal-transformations are not allowed, but
reassociation is allowed?

So the bottom line is that I do see there are issues in Clang that are relevant.
But as long as 'fast' means "'aggr' plus all the other FMF
transformations", I don't see how we can effectively disable a subset
of those other FMF transformations (while leaving 'aggr'
transformations, such as reassociation, enabled).  With that in mind, my patch
took one step in having 'fast' no longer imply all the others.

Thanks,
-Warren

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161117/e994d998/attachment-0001.html>

> ... except that Warren’s proposal that started this discussion seems to
imply that he
> has a use case that requires reciprocals to be turned off separately.
Just to close this loose end, yes I have a use case.

Specifically, we have a customer that turns on '‑ffast‑math', but was
getting a runtime failure due to the reciprocal-transformation being done.  They
don't want turn off fast‑math because they like the performance improvement,
and can live with the imprecision in most cases.  So they wanted to suppress
just the reciprocal-transformation.  I intended to tell them the solution was
simple: use '‑ffast‑math ‑fno‑reciprocal‑math'.  But on trying it
myself, I ran into the issue here.

Thanks,
-Warren

From: Kaylor, Andrew [mailto:andrew.kaylor at intel.com]
Sent: Wednesday, November 16, 2016 4:13 PM
To: Mehdi Amini <mehdi.amini at apple.com>; Ristow, Warren
<warren.ristow at sony.com>; llvm-dev at lists.llvm.org; Nicolai Hähnle
<nhaehnle at gmail.com>
Subject: RE: [llvm-dev] RFC: Consider changing the semantics of 'fast'
flag implying all fast-math-flags

I don’t really like the idea of updating checks of UnsafeAlgebra() to depend on
all of the other flags.  It seems like it would be preferable to look at each
optimization and figure out which flags it actually requires.  I suspect that in
many cases the “new” flag (i.e. allowing reassociation, etc.) will be what is
actually needed anyway.

I would be inclined to agree with Niolai’s suggestion of combining all the flags
related to value safety, except that Warren’s proposal that started this
discussion seems to imply that he has a use case that requires reciprocals to be
turned off separately.

-Andy

From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of Mehdi
Amini via llvm-dev
Sent: Wednesday, November 16, 2016 8:55 AM
To: Ristow, Warren <warren.ristow at sony.com<mailto:warren.ristow at
sony.com>>
Cc: llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
Subject: Re: [llvm-dev] RFC: Consider changing the semantics of 'fast'
flag implying all fast-math-flags


On Nov 15, 2016, at 11:59 PM, Ristow, Warren <warren.ristow at
sony.com<mailto:warren.ristow at sony.com>> wrote:

Hi,

Thanks for the quick feedback.  I see your points, but I have a few
questions/comments.  I'll start at the end of the previous post:
> ...
> I think these are valuable problems to solve, but you should tackle them
piece by piece:
>
> 1) the clang part of overriding the individual FMF and emitting the right
IR is the first thing to fix.
> 2) the backend is still using the global UnsafeFPMath and it should be
killed.
I addressed this point (2) for the reciprocal aspect in the patch, but of course
that wasn't useful without doing something about (1).

Regarding (1), over at https://reviews.llvm.org/D26708#596610, David made the
same point that it should be done in Clang.  I can understand that, but I wonder
whether having the concept of the 'fast' flag in the IR that implies all
the other FMF makes sense?  I'm not seeing a good reason for it, but since
this is very new to me, I can easily imagine I'm missing the big picture.

For example, in the LLVM IR (http://llvm.org/docs/LangRef.html#fast-math-flags)
the fast-math flags 'nnan', 'ninf', 'nsz',
'arcp' and 'fast’ are defined.  Except for 'fast', each of
these has a fairly specific definition of what they mean.  For example, for
'arcp':

    arcp => "Allow optimizations to use the reciprocal of an argument
rather
             than perform division."

'fast' is unusual, in that it describes a fairly generic set of
aggressive floating-point optimizations:

    fast => "Allow algebraically equivalent transformations that may
dramatically
            change results in floating point (e.g. reassociate). This flag
implies
            all the others."

Very loosely, 'fast' means "all the aggressive FP-transformations
that are not controlled by one of the other 4, plus it implies all the other
4".  If for terminology, we call those additional aggressive optimizations
'aggr', then we have:

    'fast' == 'aggr' + 'nnan' + 'ninf' +
'nsz' + 'arcp'

So as I see it, if we want to disable only one of the other ones (like
'arcp', in my case), there isn't any way to express that with these
IR flags defined this way.  In short, we cannot turn on all the flags besides
'arcp'.  To do that, what we want is that somehow for the Clang
switches:

  '-ffast-math -fno-reciprocal-math'

to ultimately result in LLVM IR that has the following flags on in appropriate
FP ops:

  'aggr' + 'nnan' + 'ninf' + ‘nsz'

Make sense, I missed that we can’t *subtract* from fast at the IR level.

I wouldn’t be opposed to have something along the line of “aggr”, but there is a
tradeoff: some transformation may be harder to guard with this model.

Maybe that could be a starting point: changing the “UnsafeAlgebra” bit in the
FMF to be “aggr” you mention and replace all the query to
FastMathFlags::UnsafeAlgebra() to return true if all the bits are set in the
Flags. This alone should be nothing more than a mechanical change I believe.
The important part is then auditing all the users of UnsafeAlgebra() in the
middle end and check if they can be “downgraded” to aggr safely: i.e. if they
don’t need aggr *and* another flag.

—
Mehdi







But I don't see a way to express 'aggr' in the IR.  We could do
this, if we change the definition of the IR 'fast' flag to remove that
sentence about implying all the others:

    fast => "Allow algebraically equivalent transformations that may
dramatically
            change results in floating point (e.g. reassociate).

(If we do something like that, we may want to change the name from
'fast' to something else (like 'aggr'), to avoid tying it too
closely to the concept of the '-ffast-math' switch.)

As an aside, I don't know if the "reassociate" example is the only
other transformation that's allowed by 'fast' (I presume it
isn't), but I think reassociation would be better expressed by a separate
flag, which could then be controlled independently via
'-f[no]-associative-math' switch.  Not having that flag exist separately
in the FMF is the origin of PR27372.  But creating that flag and using it in the
appropriate places would still run into these problems of 'fast'
implying all the others, which would make it impossible to disable reassociation
while leaving all the other FMF transformations enabled.

To ask a concrete question using the current definition of 'fast' (which
includes enabling reassociation, as the LLVM IR documentation of FMF says), how
can we express in the IR that reciprocal-transformations are not allowed, but
reassociation is allowed?

So the bottom line is that I do see there are issues in Clang that are relevant.
But as long as 'fast' means "'aggr' plus all the other FMF
transformations", I don't see how we can effectively disable a subset
of those other FMF transformations (while leaving 'aggr'
transformations, such as reassociation, enabled).  With that in mind, my patch
took one step in having 'fast' no longer imply all the others.

Thanks,
-Warren

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161117/a6c62711/attachment-0001.html>

> On Nov 16, 2016, at 6:22 PM, Ristow, Warren <warren.ristow at
sony.com> wrote:
> 
> > ... except that Warren’s proposal that started this discussion seems
to imply that he
> > has a use case that requires reciprocals to be turned off separately.
>  
> Just to close this loose end, yes I have a use case.
>  
> Specifically, we have a customer that turns on '‑ffast‑math', but
was getting a runtime failure due to the reciprocal-transformation being done.
Can you elaborate what kind of runtime failure is the reciprocal transformation
triggering?

— 
Mehdi

> They don't want turn off fast‑math because they like the performance
improvement, and can live with the imprecision in most cases.  So they wanted to
suppress just the reciprocal-transformation.  I intended to tell them the
solution was simple: use '‑ffast‑math ‑fno‑reciprocal‑math'.  But on
trying it myself, I ran into the issue here.
>  
> Thanks,
> -Warren
>  
> From: Kaylor, Andrew [mailto:andrew.kaylor at intel.com] 
> Sent: Wednesday, November 16, 2016 4:13 PM
> To: Mehdi Amini <mehdi.amini at apple.com>; Ristow, Warren
<warren.ristow at sony.com>; llvm-dev at lists.llvm.org; Nicolai Hähnle
<nhaehnle at gmail.com>
> Subject: RE: [llvm-dev] RFC: Consider changing the semantics of
'fast' flag implying all fast-math-flags
>  
> I don’t really like the idea of updating checks of UnsafeAlgebra() to
depend on all of the other flags.  It seems like it would be preferable to look
at each optimization and figure out which flags it actually requires.  I suspect
that in many cases the “new” flag (i.e. allowing reassociation, etc.) will be
what is actually needed anyway.
>  
> I would be inclined to agree with Niolai’s suggestion of combining all the
flags related to value safety, except that Warren’s proposal that started this
discussion seems to imply that he has a use case that requires reciprocals to be
turned off separately.
>  
> -Andy
>  <> 
>  <>From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org
<mailto:llvm-dev-bounces at lists.llvm.org>] On Behalf Of Mehdi Amini via
llvm-dev
> Sent: Wednesday, November 16, 2016 8:55 AM
> To: Ristow, Warren <warren.ristow at sony.com <mailto:warren.ristow
at sony.com>>
> Cc: llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> Subject: Re: [llvm-dev] RFC: Consider changing the semantics of
'fast' flag implying all fast-math-flags
>  
>  
> On Nov 15, 2016, at 11:59 PM, Ristow, Warren <warren.ristow at sony.com
<mailto:warren.ristow at sony.com>> wrote:
>  
> Hi,
>  
> Thanks for the quick feedback.  I see your points, but I have a few
questions/comments.  I'll start at the end of the previous post:
>  
> > ...
> > I think these are valuable problems to solve, but you should tackle
them piece by piece:
> > 
> > 1) the clang part of overriding the individual FMF and emitting the
right IR is the first thing to fix.
> > 2) the backend is still using the global UnsafeFPMath and it should be
killed.
>  
> I addressed this point (2) for the reciprocal aspect in the patch, but of
course that wasn't useful without doing something about (1).
>  
> Regarding (1), over at https://reviews.llvm.org/D26708#596610
<https://reviews.llvm.org/D26708#596610>, David made the same point that
it should be done in Clang.  I can understand that, but I wonder whether having
the concept of the 'fast' flag in the IR that implies all the other FMF
makes sense?  I'm not seeing a good reason for it, but since this is very
new to me, I can easily imagine I'm missing the big picture.
>  
> For example, in the LLVM IR
(http://llvm.org/docs/LangRef.html#fast-math-flags
<http://llvm.org/docs/LangRef.html#fast-math-flags>) the fast-math flags
'nnan', 'ninf', 'nsz', 'arcp' and 'fast’ are
defined.  Except for 'fast', each of these has a fairly specific
definition of what they mean.  For example, for 'arcp':
>  
>     arcp => "Allow optimizations to use the reciprocal of an
argument rather
>              than perform division."
>  
> 'fast' is unusual, in that it describes a fairly generic set of
aggressive floating-point optimizations:
>  
>     fast => "Allow algebraically equivalent transformations that
may dramatically
>             change results in floating point (e.g. reassociate). This flag
implies
>             all the others."
>  
> Very loosely, 'fast' means "all the aggressive
FP-transformations that are not controlled by one of the other 4, plus it
implies all the other 4".  If for terminology, we call those additional
aggressive optimizations 'aggr', then we have:
>  
>     'fast' == 'aggr' + 'nnan' + 'ninf' +
'nsz' + 'arcp'
>  
> So as I see it, if we want to disable only one of the other ones (like
'arcp', in my case), there isn't any way to express that with these
IR flags defined this way.  In short, we cannot turn on all the flags besides
'arcp'.  To do that, what we want is that somehow for the Clang
switches:
>  
>   '-ffast-math -fno-reciprocal-math'
>  
> to ultimately result in LLVM IR that has the following flags on in
appropriate FP ops:
>  
>   'aggr' + 'nnan' + 'ninf' + ‘nsz'
>  
> Make sense, I missed that we can’t *subtract* from fast at the IR level.
>  
> I wouldn’t be opposed to have something along the line of “aggr”, but there
is a tradeoff: some transformation may be harder to guard with this model.
>  
> Maybe that could be a starting point: changing the “UnsafeAlgebra” bit in
the FMF to be “aggr” you mention and replace all the query to
FastMathFlags::UnsafeAlgebra() to return true if all the bits are set in the
Flags. This alone should be nothing more than a mechanical change I believe.
> The important part is then auditing all the users of UnsafeAlgebra() in the
middle end and check if they can be “downgraded” to aggr safely: i.e. if they
don’t need aggr *and* another flag.
>  
> — 
> Mehdi
>  
>  
>  
>  
>  
>  
> 
>  
> But I don't see a way to express 'aggr' in the IR.  We could do
this, if we change the definition of the IR 'fast' flag to remove that
sentence about implying all the others:
>  
>     fast => "Allow algebraically equivalent transformations that
may dramatically
>             change results in floating point (e.g. reassociate).
>  
> (If we do something like that, we may want to change the name from
'fast' to something else (like 'aggr'), to avoid tying it too
closely to the concept of the '-ffast-math' switch.)
>  
> As an aside, I don't know if the "reassociate" example is the
only other transformation that's allowed by 'fast' (I presume it
isn't), but I think reassociation would be better expressed by a separate
flag, which could then be controlled independently via
'-f[no]-associative-math' switch.  Not having that flag exist separately
in the FMF is the origin of PR27372.  But creating that flag and using it in the
appropriate places would still run into these problems of 'fast'
implying all the others, which would make it impossible to disable reassociation
while leaving all the other FMF transformations enabled.
>  
> To ask a concrete question using the current definition of 'fast'
(which includes enabling reassociation, as the LLVM IR documentation of FMF
says), how can we express in the IR that reciprocal-transformations are not
allowed, but reassociation is allowed?
>  
> So the bottom line is that I do see there are issues in Clang that are
relevant.  But as long as 'fast' means "'aggr' plus all the
other FMF transformations", I don't see how we can effectively disable
a subset of those other FMF transformations (while leaving 'aggr'
transformations, such as reassociation, enabled).  With that in mind, my patch
took one step in having 'fast' no longer imply all the others.
>  
> Thanks,
> -Warren
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161116/8aaf1d28/attachment.html>