llvm dev - Aug 2018 - Condition code in DAGCombiner::visitFADDForFMACombine?

On 21.08.2018 16:08, Ryan Taylor via llvm-dev wrote:
> So I have a test case where:
> 
> %20 = fmul nnan arcp float %15, %19
> %21 = fadd reassoc nnan arcp contract float %20, -1.000000e+00
> 
> is being contracted in DAG to fmad. Is this correct since the fmul has 
> no reassoc or contract fast math flag?
By having the reassoc and contract flags on fadd, the frontend is 
essentially saying "different rounding on the value produced by the fadd 
is okay".

So I'd say contracting this to fma is correct.

Where does this code come from, and why do you think contracting to fma 
is wrong?

Cheers,
Nicolai



> 
> Thanks.
> 
> On Mon, Aug 20, 2018 at 12:56 PM Ryan Taylor <ryta1203 at gmail.com 
> <mailto:ryta1203 at gmail.com>> wrote:
> 
>     I'm curious why the condition to fuse is this:
> 
>     // Floating-point multiply-add with intermediate rounding.
>        bool HasFMAD = (LegalOperations &&
>     TLI.isOperationLegal(ISD::FMAD, VT));
> 
>     static bool isContractable(SDNode *N) {
>        SDNodeFlags F = N->getFlags();
>        return F.hasAllowContract() || F.hasAllowReassociation();
>     }
> 
>     bool CanFuse = Options.UnsafeFPMath || isContractable(N);
>     bool AllowFusionGlobally = (Options.AllowFPOpFusion =>    
FPOpFusion::Fast || CanFuse || HasFMAD);
>     // If the addition is not contractable, do not combine.
>     if (!AllowFusionGlobally && !isContractable(N))
>          return SDValue();
> 
>     Specifically the AllowFusionGlobally, I would have expected
>     something more like:
> 
>     bool AllowFusionGlobally = (Options.AllowFPOpFusion =>    
FPOpFusion::Fast && CanFuse && HasFMAD);
> 
>     or at the very least:
> 
>     bool AllowFusionGlobally = ((Options.AllowFPOpFusion =>    
FPOpFusion::Fast || CanFuse) && HasFMAD);
> 
>     It seems that as long as the target can do fmad it does do fmad
>     since HasFMAD is true.
> 
>     Thanks.
> 
> 
> 
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> 

-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.

The example starts as SPIR-V with the NoContraction decoration flag on the
fmul.

I think what you are saying seems valid in that if the user had put the
flag on the fadd instead of the fmul it would not contract and so in this
example the user needs to put the NoContraction on the fadd though I'm not
sure that's a good expectation of the user. On the surface, I think that if
an operation didn't have the contract flag than it wouldn't be
contracted,
regardless of what flags any other operation has.

On Wed, Aug 22, 2018 at 3:55 AM Nicolai Hähnle via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On 21.08.2018 16:08, Ryan Taylor via llvm-dev wrote:
> > So I have a test case where:
> >
> > %20 = fmul nnan arcp float %15, %19
> > %21 = fadd reassoc nnan arcp contract float %20, -1.000000e+00
> >
> > is being contracted in DAG to fmad. Is this correct since the fmul has
> > no reassoc or contract fast math flag?
>
> By having the reassoc and contract flags on fadd, the frontend is
> essentially saying "different rounding on the value produced by the
fadd
> is okay".
>
> So I'd say contracting this to fma is correct.
>
> Where does this code come from, and why do you think contracting to fma
> is wrong?
>
> Cheers,
> Nicolai
>
>
>
>
> >
> > Thanks.
> >
> > On Mon, Aug 20, 2018 at 12:56 PM Ryan Taylor <ryta1203 at gmail.com
> > <mailto:ryta1203 at gmail.com>> wrote:
> >
> >     I'm curious why the condition to fuse is this:
> >
> >     // Floating-point multiply-add with intermediate rounding.
> >        bool HasFMAD = (LegalOperations &&
> >     TLI.isOperationLegal(ISD::FMAD, VT));
> >
> >     static bool isContractable(SDNode *N) {
> >        SDNodeFlags F = N->getFlags();
> >        return F.hasAllowContract() || F.hasAllowReassociation();
> >     }
> >
> >     bool CanFuse = Options.UnsafeFPMath || isContractable(N);
> >     bool AllowFusionGlobally = (Options.AllowFPOpFusion => >    
FPOpFusion::Fast || CanFuse || HasFMAD);
> >     // If the addition is not contractable, do not combine.
> >     if (!AllowFusionGlobally && !isContractable(N))
> >          return SDValue();
> >
> >     Specifically the AllowFusionGlobally, I would have expected
> >     something more like:
> >
> >     bool AllowFusionGlobally = (Options.AllowFPOpFusion => >    
FPOpFusion::Fast && CanFuse && HasFMAD);
> >
> >     or at the very least:
> >
> >     bool AllowFusionGlobally = ((Options.AllowFPOpFusion => >   
FPOpFusion::Fast || CanFuse) && HasFMAD);
> >
> >     It seems that as long as the target can do fmad it does do fmad
> >     since HasFMAD is true.
> >
> >     Thanks.
> >
> >
> >
> >
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >
>
>
> --
> Lerne, wie die Welt wirklich ist,
> Aber vergiss niemals, wie sie sein sollte.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180822/26e45d38/attachment.html>

On 22.08.2018 13:29, Ryan Taylor wrote:
> The example starts as SPIR-V with the NoContraction decoration flag on 
> the fmul.
> 
> I think what you are saying seems valid in that if the user had put the 
> flag on the fadd instead of the fmul it would not contract and so in 
> this example the user needs to put the NoContraction on the fadd though 
> I'm not sure that's a good expectation of the user. On the surface,
I
> think that if an operation didn't have the contract flag than it 
> wouldn't be contracted, regardless of what flags any other operation
has.
Okay, I see that the SPIR-V spec specifically calls out this example.

Unless there are conflicting requirements with another frontend, I'd say 
we should make sure LLVM is aligned with SPIR-V here. Something along 
the lines of (in LangRef):

``contract``
    Allow floating-point contraction (e.g. fusing a multiply followed by
    an addition into a fused multiply-and-add). This flag must be present
    on all affected instruction.

And we should probably say the same about ``reassoc`` as well.

Cheers,
Nicolai



> 
> On Wed, Aug 22, 2018 at 3:55 AM Nicolai Hähnle via llvm-dev 
> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
> 
>     On 21.08.2018 16:08, Ryan Taylor via llvm-dev wrote:
>      > So I have a test case where:
>      >
>      > %20 = fmul nnan arcp float %15, %19
>      > %21 = fadd reassoc nnan arcp contract float %20, -1.000000e+00
>      >
>      > is being contracted in DAG to fmad. Is this correct since the
>     fmul has
>      > no reassoc or contract fast math flag?
> 
>     By having the reassoc and contract flags on fadd, the frontend is
>     essentially saying "different rounding on the value produced by
the
>     fadd
>     is okay".
> 
>     So I'd say contracting this to fma is correct.
> 
>     Where does this code come from, and why do you think contracting to fma
>     is wrong?
> 
>     Cheers,
>     Nicolai
> 
> 
> 
> 
>      >
>      > Thanks.
>      >
>      > On Mon, Aug 20, 2018 at 12:56 PM Ryan Taylor <ryta1203 at
gmail.com
>     <mailto:ryta1203 at gmail.com>
>      > <mailto:ryta1203 at gmail.com <mailto:ryta1203 at
gmail.com>>> wrote:
>      >
>      >     I'm curious why the condition to fuse is this:
>      >
>      >     // Floating-point multiply-add with intermediate rounding.
>      >        bool HasFMAD = (LegalOperations &&
>      >     TLI.isOperationLegal(ISD::FMAD, VT));
>      >
>      >     static bool isContractable(SDNode *N) {
>      >        SDNodeFlags F = N->getFlags();
>      >        return F.hasAllowContract() || F.hasAllowReassociation();
>      >     }
>      >
>      >     bool CanFuse = Options.UnsafeFPMath || isContractable(N);
>      >     bool AllowFusionGlobally = (Options.AllowFPOpFusion =>    
>     FPOpFusion::Fast || CanFuse || HasFMAD);
>      >     // If the addition is not contractable, do not combine.
>      >     if (!AllowFusionGlobally && !isContractable(N))
>      >          return SDValue();
>      >
>      >     Specifically the AllowFusionGlobally, I would have expected
>      >     something more like:
>      >
>      >     bool AllowFusionGlobally = (Options.AllowFPOpFusion =>    
>     FPOpFusion::Fast && CanFuse && HasFMAD);
>      >
>      >     or at the very least:
>      >
>      >     bool AllowFusionGlobally = ((Options.AllowFPOpFusion =>   
>     FPOpFusion::Fast || CanFuse) && HasFMAD);
>      >
>      >     It seems that as long as the target can do fmad it does do
fmad
>      >     since HasFMAD is true.
>      >
>      >     Thanks.
>      >
>      >
>      >
>      >
>      >
>      > _______________________________________________
>      > LLVM Developers mailing list
>      > llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
>      > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>      >
> 
> 
>     -- 
>     Lerne, wie die Welt wirklich ist,
>     Aber vergiss niemals, wie sie sein sollte.
>     _______________________________________________
>     LLVM Developers mailing list
>     llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>     http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> 

-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.