Re: [PATCH, midlevel]: Convert (int)floor -> lfloor, take 2

[Roger Sayle <roger at eyesopen dot com>]

On Thu, 7 Apr 2005, Uros Bizjak wrote:
> 2005-04-07  Uros Bizjak  <uros@kss-loka.si>
>
>     * builtins.def (BUILT_IN_LFLOOR, BUILT_IN_LFLOORF, BUILT_IN_LFLOORL)
>     (BUILT_IN_LLFLOOR, BUILT_IN_LLFLOORF, BUILT_IN_LLFLOORL): New.
>     * optabs.h (enum optab_index): Add new OTI_lfloor.
>     (lfloor_optab): Define corresponding macro.
>     * optabs.c (init_optabs): Initialize lfloor_optab.
>     * genopinit.c (optabs): Implement lfloor_optab using lfloorsi2
>     and lfloordi2 patterns.
>
>     * builtins.c (expand_builtin_roundingfn): New prototype.
>     (expand_builtin_roundingfn): New function.
>     (fold_builtin_roundingfn): New prototype.
>     (fold_builtin_roundingfn): New function, renamed from
>     fold_builtin_lround.
>     Handle BUILT_IN_LROUND{,F,L}, BUILT_IN_LLROUND{,F,L} and
>     BUILT_IN_LFLOOR{,F,L}, BUILT_IN_LLFLOOR{,F,L}.
>     (fold_builtin_1): Fold BUILT_IN_LFLOOR{,F,L} and
>     BUILT_IN_LLFLOOR{,F,L} using fold_builtin_roundingfn.
>     (mathfn_built_in): Handle BUILT_IN LFLOOR and BUILT_IN_LLFLOOR.
>     (expand_builtin): Expand BUILT_IN_LFLOOR{,F,L} and
>     BUILT_IN_LLFLOOR{,F,L} using expand_builtin_roundingfn.
>
>     * convert.c (convert_to_integer): Convert (long int)floor{,f,l},
>     into lfloor built-in function and (long long int)floor{,f,l} into
>     llfloor built-in-function.
>     * fold-const.c (tree_expr_nonnegative_p): Add BUILT_IN_LFLOOR and
>     BUILT_IN_LLFLOOR.
>
> testsuite:
>
>     * gcc.dg/builtins-53.c: New test.

Hi Uros,

Everything is looking good except that expand_builtin_roundingfn needs
some minor tweaks.  I hope you don't mind, but I think this needs another
iteration.

Firstly, I think we should define the semantics of the new builtins
__builtin_lfloor to match the original (int)floor(x).  Firstly, this
means that unlike "lround" and "lrint", they don't/shouldn't set errno.

This is important because if flag_errno_math is true, we have absolutely
no way of detecting that an EDOM has occured from the integer result.
For most <math.h> functions, that return floating point values, we can
test whether an exceptional/error condition has occured by inspecting
the result for NaN, in which case we can call libm with the original
arguments to detect to correctly set errno for us.

Unfortunately, for floating point to integer conversions we return an
integral result, or an "undefined" result, hence we have no way after
the fact of checking whether the original operand was a NaN.  Hence
all of the errno_set code in your new expand_builtin_roundingfn is
problematic.  Instead, we should realise/observe that in the original
source (int)floor(x) can never set errno!  Hence, we should define
you new builtins with the attributes ATTR_MATHFN_FPROUNDING instead of
ATTR_MATHFN_ERRNO in builtins.def.

This in turn simplifies things even more as the new RTL expander no
longer has to worry about having it's result ignored (i.e. target
== const0_rtx).  Once, these conversion functions are marked as
const/pure, the main routine expand_builtin will take care of things
for us, if our result is never required.


You're right that you don't need a start_sequence()/end_sequence()
around expand_fix as at this point you've already committed to
emitting these instructions at the end of the instruction stream.
The use of sequence is normally reserved for cases where an expansion
may fail and we wan't to avoid insns leaking from a failed expansion
attempt.  In this case, expand_fix can't fail, and all of the insns
it emits are always required.

Then finally, I don't really like the way you've implemented the
fallback path, by attempting to "goto fallback:" at the top of
the function, and then having to check/differentiate the fallback
case using "if (builtin_optab != fallback_optab)".

Instead, I think it would be better to avoid the uses of goto,
and instead recursively invoke "expand_expr" or "expand_builtin_mathfn"
to perform the lowering for you.  Currently you're tieing your routine
in knots jumping backwards and forwards in an attempt to do everything
directly.

To summarise, all of this:

+  /* Fall back to floating point rounding optab.  */
+  if (builtin_optab != fallback_optab)
+    {
+      tree fallback_fndecl;
+
+      target_mode = mode;
+      mode = TYPE_MODE (TREE_TYPE (arg));
+
+      fallback_fndecl = mathfn_built_in (TREE_TYPE (arg), fallback_fn);
+      exp = build_function_call_expr (fallback_fndecl, arglist);
+
+      builtin_optab = fallback_optab;
+      goto fallback;
+    }
+
+  target = expand_call (exp, target, ignore);
+
+ fallback_exit:
...

Becomes something like:

+      fallback_fndecl = mathfn_built_in (TREE_TYPE (arg), fallback_fn);
+      exp = build_function_call_expr (fallback_fndecl, arglist);
+      target = expand_builtin (exp, NULL_RTX, NULL_RTX, mode, false);
+      expand_fix (tmp, target, 0);

For example, notice that we don't need fallback_optab.  As long as
we know the fallback_fn, it'll be the recursive invocation's job to
handle the optabs, etc, etc...


[Infact, by doing a Kazu-like reorganization of expand_builtin_mathfn
to take an fndecl, type and arglist, it's even possible to avoid
creating the temporary CALL_EXPR above.  This really isn't an issue,
but I mention it as a possible follow-up clean-up.]


The shape of your patch looks good, its just that RTL expansion is even
easier than you've implemented.  Feel free to repost just the revised
expand_builtin_roundingfn for comment, if you're not confident about or
don't understand any of the suggestions above (the builtins.def change
described earlier is obvious, and all your other changes are fine).

Many thanks for your patience.

Roger
--