Nicolai Hähnle via llvm-dev
2018-Aug-22 20:33 UTC
[llvm-dev] Condition code in DAGCombiner::visitFADDForFMACombine?
On 22.08.2018 17:52, Ryan Taylor wrote:> This is probably going to effect on other backends and break llvm-lit > for them?Very likely, yes. Can you take a look at how big the fallout is? This might give us a hint about what other frontends might expect, and who needs to be involved in the discussion (if one is needed). Cheers, Nicolai> > On Wed, Aug 22, 2018 at 11:41 AM Nicolai Hähnle <nhaehnle at gmail.com > <mailto:nhaehnle at gmail.com>> wrote: > > 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> > <mailto: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>> > > > <mailto: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> > <mailto: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> > <mailto: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. >-- Lerne, wie die Welt wirklich ist, Aber vergiss niemals, wie sie sein sollte.
Ryan Taylor via llvm-dev
2018-Aug-22 21:53 UTC
[llvm-dev] Condition code in DAGCombiner::visitFADDForFMACombine?
Just a quick change of isContractableFMUL to isContractable gives about 49 unexpected failures. ******************** Testing Time: 391.60s ******************** Failing Tests (49): LLVM :: CodeGen/AArch64/neon-fma-FMF.ll LLVM :: CodeGen/AMDGPU/clamp-modifier.ll LLVM :: CodeGen/AMDGPU/fadd-fma-fmul-combine.ll LLVM :: CodeGen/AMDGPU/fcanonicalize-elimination.ll LLVM :: CodeGen/AMDGPU/fma-combine.ll LLVM :: CodeGen/AMDGPU/fmad.ll LLVM :: CodeGen/AMDGPU/fmul-2-combine-multi-use.ll LLVM :: CodeGen/AMDGPU/fmuladd.f16.ll LLVM :: CodeGen/AMDGPU/fmuladd.f32.ll LLVM :: CodeGen/AMDGPU/fmuladd.f64.ll LLVM :: CodeGen/AMDGPU/fneg-combines.ll LLVM :: CodeGen/AMDGPU/fpext-free.ll LLVM :: CodeGen/AMDGPU/llvm.cos.ll LLVM :: CodeGen/AMDGPU/llvm.fmuladd.f16.ll LLVM :: CodeGen/AMDGPU/llvm.sin.ll LLVM :: CodeGen/AMDGPU/mad-combine.ll LLVM :: CodeGen/AMDGPU/mad-mix-hi.ll LLVM :: CodeGen/AMDGPU/mad-mix-lo.ll LLVM :: CodeGen/AMDGPU/mad-mix.ll LLVM :: CodeGen/AMDGPU/madak.ll LLVM :: CodeGen/AMDGPU/madmk.ll LLVM :: CodeGen/AMDGPU/omod.ll LLVM :: CodeGen/AMDGPU/operand-folding.ll LLVM :: CodeGen/AMDGPU/pv-packing.ll LLVM :: CodeGen/AMDGPU/reduction.ll LLVM :: CodeGen/AMDGPU/sdwa-peephole.ll LLVM :: CodeGen/AMDGPU/shared-op-cycle.ll LLVM :: CodeGen/AMDGPU/v_mac.ll LLVM :: CodeGen/AMDGPU/v_mac_f16.ll LLVM :: CodeGen/AMDGPU/v_madak_f16.ll LLVM :: CodeGen/Hexagon/float-amode.ll LLVM :: CodeGen/Hexagon/fmadd.ll LLVM :: CodeGen/Hexagon/fp_latency.ll LLVM :: CodeGen/NVPTX/fma-assoc.ll LLVM :: CodeGen/PowerPC/a2-fp-basic.ll LLVM :: CodeGen/PowerPC/fma-aggr-FMF.ll LLVM :: CodeGen/PowerPC/fma-assoc.ll LLVM :: CodeGen/PowerPC/fma-ext.ll LLVM :: CodeGen/PowerPC/fma.ll LLVM :: CodeGen/PowerPC/fmf-propagation.ll LLVM :: CodeGen/PowerPC/ppc440-fp-basic.ll LLVM :: CodeGen/PowerPC/qpx-recipest.ll LLVM :: CodeGen/PowerPC/recipest.ll LLVM :: CodeGen/X86/avx512-fma.ll LLVM :: CodeGen/X86/fma-do-not-commute.ll LLVM :: CodeGen/X86/fma_patterns.ll LLVM :: CodeGen/X86/fma_patterns_wide.ll LLVM :: CodeGen/X86/sqrt-fastmath-mir.ll LLVM :: CodeGen/X86/sqrt-fastmath.ll Expected Passes : 25605 Expected Failures : 149 Unsupported Tests : 676 Unexpected Failures: 49 On Wed, Aug 22, 2018 at 4:33 PM Nicolai Hähnle <nhaehnle at gmail.com> wrote:> On 22.08.2018 17:52, Ryan Taylor wrote: > > This is probably going to effect on other backends and break llvm-lit > > for them? > > Very likely, yes. Can you take a look at how big the fallout is? This > might give us a hint about what other frontends might expect, and who > needs to be involved in the discussion (if one is needed). > > Cheers, > Nicolai > > > > > > On Wed, Aug 22, 2018 at 11:41 AM Nicolai Hähnle <nhaehnle at gmail.com > > <mailto:nhaehnle at gmail.com>> wrote: > > > > 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> > > <mailto: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>> > > > > <mailto: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> > > <mailto: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> > > <mailto: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. > > > > > -- > Lerne, wie die Welt wirklich ist, > Aber vergiss niemals, wie sie sein sollte. >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180822/5c95e89d/attachment.html>
Nicolai Hähnle via llvm-dev
2018-Aug-23 14:47 UTC
[llvm-dev] Condition code in DAGCombiner::visitFADDForFMACombine?
On 22.08.2018 23:53, Ryan Taylor wrote:> Just a quick change of isContractableFMUL to isContractable gives about > 49 unexpected failures.That may not be the right change to be looking at. Consider:> LLVM :: CodeGen/X86/fma_patterns.llThis file doesn't use any `contract` or `reassoc` flags on IR instructions. I don't know what's changed there, but the point is that the test does not depend on the exact semantics of `contract` and `reassoc`. Cheers, Nicolai -- Lerne, wie die Welt wirklich ist, Aber vergiss niemals, wie sie sein sollte.