On Tue, Oct 19, 2021 at 9:03 AM Prathamesh Kulkarni via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Mon, 18 Oct 2021 at 17:23, Richard Biener <rguent...@suse.de> wrote:
> >
> > On Mon, 18 Oct 2021, Prathamesh Kulkarni wrote:
> >
> > > On Mon, 18 Oct 2021 at 17:10, Richard Biener <rguent...@suse.de> wrote:
> > > >
> > > > On Mon, 18 Oct 2021, Prathamesh Kulkarni wrote:
> > > >
> > > > > On Mon, 18 Oct 2021 at 16:18, Richard Biener <rguent...@suse.de>
> > > > > wrote:
> > > > > >
> > > > > > On Mon, 18 Oct 2021, Prathamesh Kulkarni wrote:
> > > > > >
> > > > > > > Hi Richard,
> > > > > > > As suggested in PR, I have attached WIP patch that adds two
> > > > > > > patterns
> > > > > > > to match.pd:
> > > > > > > erfc(x) --> 1 - erf(x) if canonicalize_math_p() and,
> > > > > > > 1 - erf(x) --> erfc(x) if !canonicalize_math_p().
> > > > > > >
> > > > > > > This works to remove call to erfc for the following test:
> > > > > > > double f(double x)
> > > > > > > {
> > > > > > > double g(double, double);
> > > > > > >
> > > > > > > double t1 = __builtin_erf (x);
> > > > > > > double t2 = __builtin_erfc (x);
> > > > > > > return g(t1, t2);
> > > > > > > }
> > > > > > >
> > > > > > > with .optimized dump shows:
> > > > > > > t1_2 = __builtin_erf (x_1(D));
> > > > > > > t2_3 = 1.0e+0 - t1_2;
> > > > > > >
> > > > > > > However, for the following test:
> > > > > > > double f(double x)
> > > > > > > {
> > > > > > > double g(double, double);
> > > > > > >
> > > > > > > double t1 = __builtin_erfc (x);
> > > > > > > return t1;
> > > > > > > }
> > > > > > >
> > > > > > > It canonicalizes erfc(x) to 1 - erf(x), but does not transform 1 -
> > > > > > > erf(x) to erfc(x) again
> > > > > > > post canonicalization.
> > > > > > > -fdump-tree-folding shows that 1 - erf(x) --> erfc(x) gets
> > > > > > > applied,
> > > > > > > but then it tries to
> > > > > > > resimplify erfc(x), which fails post canonicalization. So we end
> > > > > > > up
> > > > > > > with erfc(x) transformed to
> > > > > > > 1 - erf(x) in .optimized dump, which I suppose isn't ideal.
> > > > > > > Could you suggest how to proceed ?
> > > > > >
> > > > > > I applied your patch manually and it does the intended
> > > > > > simplifications so I wonder what I am missing?
> > > > > Would it be OK to always fold erfc(x) -> 1 - erf(x) even when there's
> > > > > no erf(x) in the source ?
> > > >
> > > > I do think it's reasonable to expect erfc to be available when erf
> > > > is and vice versa but note both are C99 specified functions (either
> > > > requires -lm).
> > > OK, thanks. Would it be OK to commit the patch after bootstrap+test ?
> >
> > Yes, but I'm confused because you say the patch doesn't work for you?
> The patch works for me to CSE erf/erfc pair.
> However when there's only erfc in the source, it canonicalizes erfc(x)
> to 1 - erf(x) but later fails to uncanonicalize 1 - erf(x) back to
> erfc(x)
> with -O3 -funsafe-math-optimizations.
>
> For,
> t1 = __builtin_erfc(x),
>
> .optimized dump shows:
> _2 = __builtin_erf (x_1(D));
> t1_3 = 1.0e+0 - _2;
>
> and for,
> double t1 = x + __builtin_erfc(x);
>
> .optimized dump shows:
> _3 = __builtin_erf (x_2(D));
> _7 = x_2(D) + 1.0e+0;
> t1_4 = _7 - _3;
>
> I assume in both cases, we want erfc in the code-gen instead ?
> I think the reason uncaonicalization fails is because the pattern 1 -
> erf(x) to erfc(x)
> gets applied, but then it fails in resimplifying erfc(x), and we end
> up with 1 - erf(x) in code-gen.
>
> From gimple-match.c, it hits the simplification:
>
> gimple_seq *lseq = seq;
> if (__builtin_expect (!dbg_cnt
> (match), 0)) goto next_after_fail1172;
> if (__builtin_expect (dump_file &&
> (dump_flags & TDF_FOLDING), 0)) fprintf (dump_file, "Applying pattern
> %s:%d, %s:%d\n", "match.pd", 6162, __FILE__, __LINE__);
> {
> res_op->set_op (CFN_BUILT_IN_ERFC, type, 1);
> res_op->ops[0] = captures[0];
> res_op->resimplify (lseq, valueize);
> return true;
> }
>
> But res_op->resimplify returns false, and doesn't end up adding to lseq.
There's nothing to add to lseq since there's also nothing to resimplify.
The only thing that could happen is that the replacement is not done
because replace_stmt_with_simplification via maybe_push_res_to_seq
doesn't pass the builtin_decl_implicit test:
/* Find the function we want to call. */
tree decl = builtin_decl_implicit (as_builtin_fn (fn));
if (!decl)
return NULL;
btw, it did work for me since the call was present before and gimplification
should then mark the function eligible for implicit generation.
> As you suggest, should we instead handle this in fre to transform
> erfc(x) to 1 - erf(x), only when
> there's a matching erf(x) in the source ?
Note that's strictly less powerful and we'd have to handle erf(x) -> 1 +erfc (x)
to handle CSE in
tem = erfc (x);
tem2 = erf (x);
So no, I think the canonicalization is fine unless there's a compelling reason
for having both erfc and erf.
Can you debug why the reverse transform doesn't work for you?
Richard.
> Thanks,
> Prathamesh
> >
> > Btw, please add the testcase from the PR and also a testcase that shows
> > the canonicalization is undone. Maybe you can also double-check that
> > we handle x + erfc (x) because I see we associate that as
> > (x + 1) - erf (x) which is then not recognized back to erfc.
> >
> > The less surprising (as to preserve the function called in the source)
> > variant for the PR would be to teach CSE to lookup erf(x) when
> > visiting erfc(x) and when found synthesize 1 - erf(x).
> >
> > That said, a mathematician should chime in on how important it is
> > to preserve erfc vs. erf (precision or even speed).
> >
> > Thanks,
> > Richard.
> >
> > > Thanks,
> > > Prathamesh
> > >
> > > >
> > > > Richard.
> > > >
> > > > > So for the following test:
> > > > > double f(double x)
> > > > > {
> > > > > t1 = __builtin_erfc(x)
> > > > > return t1;
> > > > > }
> > > > >
> > > > > .optimized dump shows:
> > > > > double f (double x)
> > > > > {
> > > > > double t1;
> > > > > double _2;
> > > > >
> > > > > <bb 2> [local count: 1073741824]:
> > > > > _2 = __builtin_erf (x_1(D));
> > > > > t1_3 = 1.0e+0 - _2;
> > > > > return t1_3;
> > > > > }
> > > > >
> > > > > while before patch, it has:
> > > > > t1_4 = __builtin_erfc (x_2(D)); [tail call]
> > > > > return t1_4;
> > > > >
> > > > > Thanks,
> > > > > Prathamesh
> > > > >
> > > > > >
> > > > > > Richard.
> > > > > >
> > > > > > > Thanks,
> > > > > > > Prathamesh
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > Richard Biener <rguent...@suse.de>
> > > > > > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409
> > > > > > Nuernberg,
> > > > > > Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
> > > > >
> > > >
> > > > --
> > > > Richard Biener <rguent...@suse.de>
> > > > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> > > > Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
> > >
> >
> > --
> > Richard Biener <rguent...@suse.de>
> > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> > Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
