Re: [PATCH] fix possible FP reassociation problem in simplify-rtx.c

[Roger Sayle <roger at eyesopen dot com>]

Hi Paolo,

On Thu, 24 Nov 2005, Paolo Bonzini wrote:
> 2005-11-23  Paolo Bonzini  <bonzini@gnu.org>
>
>	* simplify-rtx.c (simplify_plus_minus): Remove final parameter.
>	Always produce an output if we can remove NEGs or canonicalize
>	(minus (minus ...)) expressions.  Provide a fast path for the
>	two-operand case.
>	(simplify_gen_binary): Do not call simplify_plus_minus.
>	(simplify_binary_operation_1): Reassociate at the end of the
>	function.
...


> -  /* If we only have two operands, we can't do anything.  */
> -  if (n_ops <= 2 && !force)
> +  gcc_assert (n_ops >= 2);
> +  if (!canonicalized)
>      return NULL_RTX;

I'll let you decide whether its necessary to have a gcc_assert here.
The initial value of n_ops is two, and its only ever incremented,
(upon encounting a binary operator in a single RTX).  i.e. it can't
overflow and can't ever decrease.  The previous condition was just
a defensive form of "n_ops == 2".


> +      else if (ops[0].neg)
> +	lhs = ops[1].op, rhs = ops[0].op;
> +      else
> +	lhs = ops[1].op, rhs = ops[0].op;

Typo here.  The second line should be written as

	lhs = ops[0].op, rhs = ops[1].op;

Personally, I'm not a great fan of using commas to conjoin statements,
so I'd probably write this myself as

	else if (ops[0].neg)
	  {
	    lhs = ops[1].op;
	    rhs = ops[0].op;
	  }
	else
	  {
	    lhs = ops[0].op;
	    rhs = ops[1].op;
	  }

But I appreciate that this is just a personal preference.

Ok for mainline with the correction to the typo.


Was this change to disable floating point reassociation inspired
by a real test case, or from inspection of the code?  It might be
reasonable to continue to allow reassociation of FP operations with
flag_unsafe_math_optimizations.

Thanks, and congratulations for getting an observable speed-up on
SPEC with this change.

Roger
--