This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH]: Add some folding of builtins fmin/fmax
- From: Roger Sayle <roger at eyesopen dot com>
- To: "Kaveh R. GHAZI" <ghazi at caip dot rutgers dot edu>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Sat, 25 Nov 2006 11:16:50 -0700 (MST)
- Subject: Re: [PATCH]: Add some folding of builtins fmin/fmax
On Sat, 25 Nov 2006, Kaveh R. GHAZI wrote:
> 2006-11-23 Kaveh R. Ghazi <email@example.com>
> * builtins.c (fold_builtin_fmin_fmax): Handle NaN arguments.
> * gcc.dg/torture/builtin-minmax-1.c: Test NaN in fmin/fmax.
This is OK for mainline. Thanks. You're prefectly correct that
you don't need to use omit_one_operand here, as a NaN REAL_CST doesn't
have any explicit side-effects, but we might consider disabling
this transformation when we honor flag_signaling_nans, i.e. HONOR_SNANS.
Not sure what the prescribed signaling NaN semantics for fmin/fmax are,
but it does no harm to skip this optimization if someone explicitly
passes -fsignaling-nans on the command line.
> + /* If either argument is NaN, return the other one. */
> + if (TREE_CODE (arg0) == REAL_CST && real_isnan (&TREE_REAL_CST (arg0)))
> + return fold_convert (type, arg1);
> + else
> + if (TREE_CODE (arg1) == REAL_CST && real_isnan (&TREE_REAL_CST (arg1)))
> + return fold_convert (type, arg0);
I'd also prefer removing the "else" here. The prevailing style in
fold-const.c/builtins.c is to list the long sequence of possible
transformations omitting the "else" between independent transformations
that can be reordered, especially when the then-clause always returns.
This is easier to read than the deep else-if constructs in cse.c and
combine.c and also slightly easier on the compiler/parser, c.f. PR12454.
> /* Transform fmin/fmax(x,x) -> x. */
> if (operand_equal_p (arg0, arg1, OEP_PURE_SAME))
> return omit_one_operand (type, arg0, arg1);
In fact, I think we don't strictly need omit_one_operand here as
operand_equal_p guarantees that that its arguments don't have
side-effects when it returns true. But this is a pre-existing
condition, that I just noticed in reference to Andrew's comments.
Thanks for implementing this. I'll also pre-approve either a revised
patch or a follow-up patch that addresses the above issues. Thanks