Re: [Committed] ifcvt.c solution to PR9814

[Roger Sayle <roger at eyesopen dot com>]

Hi Richard,

Thanks again.

On Sat, 28 May 2005, Richard Henderson wrote:
> Hmm.  How about this rearrangement:
>   ...
>   else if (OBJECT_P (y) || CONSTANT_P (y) || GET_CODE (y) == SUBREG)
>     emit_move_insn (x, y)
>   else
>     {
>       emit_insn (gen_rtx_SET (VOIDmode, x, y));
>       ...
>     }

There's just one aspect of your suggestion that I don't fully understand.
As shown by PR rtl-optimization/20369, that even before my "suspicious"
use of emit_move_insn in noce_emit_move_insn, it was possible that the
insn returned by emit_move_insn wasn't recognizable.  This should have
been cured by my recent changes as we cautiously use a sequence and
manually attempt to recognize the result by hand.

Now whilst I understand and completely agree with your comment:

> I'm thinking of the situation in which we have a rhs constant that's
> too large for a direct set, but for which the mov<mode> pattern would
> have expanded to a pair of sets.

i.e. we should emit_move_insn when we can, rather than take the
simplistic approach of always building the SET pattern ourselves.


My confusion with the code fragment you proposed was that it looked
like you imply that by checking "OBJECT_P (y) || CONSTANT_P (y) || ..."
that we can now guarantee that the resulting insn must always be
recognizable?


I've absolutely no idea whether this is the case or not, or just an
artifact of the way that your pseudo code appeared in the e-mail.

Erring on the side of caution, the patch below continues to assume
that emit_move_insn may potentially return an unrecognizable insn.
This makes for a shorter patch, and is safe if overly paranoid.  If
you're certain that this check isn't needed with the "general_operand"
tests, let me know and I'll prepare a patch closer to your previous
suggestion.


Sorry, if I over interpreted your suggested psuedo-code.  By accidentally
ommitting the start_sequence from the emit_insn case, it isn't clear
whether you've intentionally ommitted it (and the following ...) from
the emit_move_insn case.

Does that/my confusion make any sense?



The following patch implements the spirit of your recommendation.  It's
been tested on i686-pc-linux-gnu with a full "make bootsrap", all default
languages, and regression tested with a top-level "make -k check" with
no new failures.

Ok for mainline?


2005-05-28  Roger Sayle  <roger@eyesopen.com>
	    Richard Henderson  <rth@redhat.com>

	* ifcvt.c (noce_emit_move_insn): Construct a SET pattern directly
	if the RHS isn't suitable for calling emit_move_insn.


Index: ifcvt.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/ifcvt.c,v
retrieving revision 1.188
diff -c -3 -p -r1.188 ifcvt.c
*** ifcvt.c	27 May 2005 02:45:55 -0000	1.188
--- ifcvt.c	28 May 2005 23:06:48 -0000
*************** noce_emit_move_insn (rtx x, rtx y)
*** 691,697 ****
        optab ot;

        start_sequence ();
!       insn = emit_move_insn (x, y);
        seq = get_insns ();
        end_sequence();

--- 691,701 ----
        optab ot;

        start_sequence ();
!       /* Check that the SET_SRC is reasonable before calling emit_move_insn,
! 	 otherwise construct a suitable SET pattern ourselves.  */
!       insn = (OBJECT_P (y) || CONSTANT_P (y) || GET_CODE (y) == SUBREG)
! 	     ? emit_move_insn (x, y)
! 	     : emit_insn (gen_rtx_SET (VOIDmode, x, y));
        seq = get_insns ();
        end_sequence();



Roger
--