On Mon, Nov 5, 2018 at 3:11 PM Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> wrote: > > On Mon, 5 Nov 2018 at 18:14, Richard Biener <richard.guent...@gmail.com> > wrote: > > > > On Mon, Nov 5, 2018 at 1:11 PM Prathamesh Kulkarni > > <prathamesh.kulka...@linaro.org> wrote: > > > > > > On Mon, 5 Nov 2018 at 15:10, Richard Biener <richard.guent...@gmail.com> > > > wrote: > > > > > > > > On Fri, Nov 2, 2018 at 10:37 AM Prathamesh Kulkarni > > > > <prathamesh.kulka...@linaro.org> wrote: > > > > > > > > > > Hi, > > > > > This patch adds two transforms to match.pd to CSE erf/erfc pair. > > > > > erfc(x) is canonicalized to 1 - erf(x) and is then reversed to 1 - > > > > > erf(x) when canonicalization is disabled and result of erf(x) has > > > > > single use within 1 - erf(x). > > > > > > > > > > The patch regressed builtin-nonneg-1.c. The following test-case > > > > > reproduces the issue with patch: > > > > > > > > > > void test(double d1) { > > > > > if (signbit(erfc(d1))) > > > > > link_failure_erfc(); > > > > > } > > > > > > > > > > ssa dump: > > > > > > > > > > <bb 2> : > > > > > _5 = __builtin_erf (d1_4(D)); > > > > > _1 = 1.0e+0 - _5; > > > > > _6 = _1 < 0.0; > > > > > _2 = (int) _6; > > > > > if (_2 != 0) > > > > > goto <bb 3>; [INV] > > > > > else > > > > > goto <bb 4>; [INV] > > > > > > > > > > <bb 3> : > > > > > link_failure_erfc (); > > > > > > > > > > <bb 4> : > > > > > return; > > > > > > > > > > As can be seen, erfc(d1) is folded to 1 - erf(d1). > > > > > forwprop then transforms the if condition from _2 != 0 > > > > > to _5 > 1.0e+0 and that defeats DCE thus resulting in link failure > > > > > in undefined reference to link_failure_erfc(). > > > > > > > > > > So, the patch adds another transform erf(x) > 1 -> 0. > > > > > > > > Ick. > > > > > > > > Why not canonicalize erf (x) to 1-erfc(x) instead? > > > Sorry I didn't quite follow, won't this cause similar issue with erf ? > > > I changed the pattern to canonicalize erf(x) -> 1 - erfc(x) > > > and 1 - erfc(x) -> erf(x) after canonicalization is disabled. > > > > > > This caused undefined reference to link_failure_erf() in following > > > test-case: > > > > > > extern int signbit(double); > > > extern void link_failure_erf(void); > > > extern double erf(double); > > > > > > void test(double d1) { > > > if (signbit(erf(d1))) > > > link_failure_erf(); > > > } > > > > But that's already not optimized without any canonicalization > > because erf returns sth in range [-1, 1]. > > > > I suggested the change because we have limited support for FP > > value-ranges and nonnegative is one thing we can compute > > (and erfc as opposed to erf is nonnegative). > Ah right, thanks for the explanation. > Unfortunately this still regresses builtin-nonneg-1.c, which can be > reproduced with following test-case: > > extern int signbit(double); > extern void link_failure_erf(void); > extern double erf(double); > extern double fabs(double); > > void test(double d1) { > if (signbit(erf(fabs(d1)))) > link_failure_erf(); > } > > signbit(erf(fabs(d1)) is transformed to 0 without patch but with patch > it gets canonicalized to signbit(1 - erfc(fabs(d1))) which similarly > defeats DCE. > > forwprop1 shows: > <bb 2> : > _1 = ABS_EXPR <d1_5(D)>; > _6 = __builtin_erfc (_1); > _2 = 1.0e+0 - _6; > _7 = _6 > 1.0e+0; > _3 = (int) _7; > if (_6 > 1.0e+0) > goto <bb 3>; [INV] > else > goto <bb 4>; [INV] > > <bb 3> : > link_failure_erf (); > > <bb 4> : > return; > > I assume we would need to somehow tell gcc that the canonicalized > expression 1 - erfc(x) would not exceed 1.0 ? > Is there a better way to do that apart from defining pattern (1 - > erfc(x)) > 1.0 -> 0 > which I agree doesn't look ideal to add in match.pd ?
You could handle a MINUS_EXPR of 1 and erfc() in gimple_assign_nonnegative_warnv_p (I wouldn't bother to do it for tree_binary_nonnegative_warnv_p) This is of course similarly "lame" but a bit cleaner than a match.pd pattern that just works for the comparison. In reality the proper long-term fix is to add basic range propagation to floats. Richard. > > Thanks > Prathamesh > > > > > forwprop1 shows: > > > > > > <bb 2> : > > > _5 = __builtin_erfc (d1_4(D)); > > > _1 = 1.0e+0 - _5; > > > _6 = _5 > 1.0e+0; > > > _2 = (int) _6; > > > if (_5 > 1.0e+0) > > > goto <bb 3>; [INV] > > > else > > > goto <bb 4>; [INV] > > > > > > <bb 3> : > > > link_failure_erf (); > > > > > > <bb 4> : > > > return; > > > > > > which defeats DCE to remove call to link_failure_erf. > > > > > > Thanks, > > > Prathamesh > > > > > > > > > which resolves the regression. > > > > > > > > > > Bootstrapped+tested on x86_64-unknown-linux-gnu. > > > > > Cross-testing on arm and aarch64 variants in progress. > > > > > OK for trunk if passes ? > > > > > > > > > > Thanks, > > > > > Prathamesh