Fix PR69291, RTL if-conversion bug

On Wed, 10 Feb 2016, Richard Biener wrote:

> On Wed, 10 Feb 2016, Bernd Schmidt wrote:
> 
> > On 02/10/2016 02:04 PM, Richard Biener wrote:
> > > where noce_try_store_flag_constants identifies
> > > 
> > > (plus:SI (reg/v:SI 160 [ mod_tlen ])
> > >           (reg/v:SI 224 [ <retval> ]))
> > > 
> > > as "common" and then tries to detect the case where setting the
> > > result would clobber that value.  It doesn't seem to expect
> > > anything else than regs that can be equal to the destination though
> > > which is clearly an oversight.
> > 
> > >         /* If we have x := test ? x + 3 : x + 4 then move the original
> > >   	 x out of the way while we store flags.  */
> > > -      if (common && rtx_equal_p (common, if_info->x))
> > > +      if (common && reg_mentioned_p (if_info->x, common))
> > >   	{
> > > -	  common = gen_reg_rtx (mode);
> > > -	  noce_emit_move_insn (common, if_info->x);
> > > +	  rtx tem = gen_reg_rtx (mode);
> > > +	  noce_emit_move_insn (tem, common);
> > > +	  common = tem;
> > >   	}
> > 
> > I'm not so sure noce_emit_move_insn will reliably handle an arbitrary
> > expression. I think a more conservative fix would be to disable this transform
> > if common is not a reg.
> 
> I also wondered about this but then noce_emit_move_insn is quite elaborate
> (calling into expanders eventually).
> 
> But if you prefer I can instead test the following
> 
> Index: gcc/ifcvt.c
> ===================================================================
> --- gcc/ifcvt.c (revision 233262)
> +++ gcc/ifcvt.c (working copy)
> @@ -1274,7 +1274,7 @@ noce_try_store_flag_constants (struct no
>        && CONST_INT_P (XEXP (a, 1))
>        && CONST_INT_P (XEXP (b, 1))
>        && rtx_equal_p (XEXP (a, 0), XEXP (b, 0))
> -      && noce_operand_ok (XEXP (a, 0))
> +      && REG_P (XEXP (a, 0))
>        && if_info->branch_cost >= 2)
>      {
>        common = XEXP (a, 0);

Or less aggressive

On 02/10/2016 02:35 PM, Richard Biener wrote:

> Index: gcc/ifcvt.c
> ===================================================================
> --- gcc/ifcvt.c (revision 233262)
> +++ gcc/ifcvt.c (working copy)
> @@ -1274,7 +1274,8 @@ noce_try_store_flag_constants (struct no
>         && CONST_INT_P (XEXP (a, 1))
>         && CONST_INT_P (XEXP (b, 1))
>         && rtx_equal_p (XEXP (a, 0), XEXP (b, 0))
> -      && noce_operand_ok (XEXP (a, 0))
> +      && (REG_P (XEXP (a, 0))
> +         || ! reg_mentioned_p (if_info->x, XEXP (a, 0)))

I guess that would also work. Could maybe use a brief comment.


Bernd

On Wed, 10 Feb 2016, Bernd Schmidt wrote:

> On 02/10/2016 02:35 PM, Richard Biener wrote:
> 
> > Index: gcc/ifcvt.c
> > ===================================================================
> > --- gcc/ifcvt.c (revision 233262)
> > +++ gcc/ifcvt.c (working copy)
> > @@ -1274,7 +1274,8 @@ noce_try_store_flag_constants (struct no
> >         && CONST_INT_P (XEXP (a, 1))
> >         && CONST_INT_P (XEXP (b, 1))
> >         && rtx_equal_p (XEXP (a, 0), XEXP (b, 0))
> > -      && noce_operand_ok (XEXP (a, 0))
> > +      && (REG_P (XEXP (a, 0))
> > +         || ! reg_mentioned_p (if_info->x, XEXP (a, 0)))
> 
> I guess that would also work. Could maybe use a brief comment.

Ok.  I'm testing that.  I wonder if we need to use reg_overlap_mentioned_p
here (hard-reg pairs?) or if reg_mentioned_p is safe.

I'm not too much into RTL ...

Thanks,
Richard.

On 02/10/2016 02:50 PM, Richard Biener wrote:
> On Wed, 10 Feb 2016, Bernd Schmidt wrote:
>
>> On 02/10/2016 02:35 PM, Richard Biener wrote:
>>
>>> Index: gcc/ifcvt.c
>>> ===================================================================
>>> --- gcc/ifcvt.c (revision 233262)
>>> +++ gcc/ifcvt.c (working copy)
>>> @@ -1274,7 +1274,8 @@ noce_try_store_flag_constants (struct no
>>>          && CONST_INT_P (XEXP (a, 1))
>>>          && CONST_INT_P (XEXP (b, 1))
>>>          && rtx_equal_p (XEXP (a, 0), XEXP (b, 0))
>>> -      && noce_operand_ok (XEXP (a, 0))
>>> +      && (REG_P (XEXP (a, 0))
>>> +         || ! reg_mentioned_p (if_info->x, XEXP (a, 0)))
>>
>> I guess that would also work. Could maybe use a brief comment.
>
> Ok.  I'm testing that.  I wonder if we need to use reg_overlap_mentioned_p
> here (hard-reg pairs?) or if reg_mentioned_p is safe.

Let's go with reg_overlap_mentioned_p. I kind of forgot about that once 
I thought of possible issues with emitting a move :-(


Bernd