RFA: Fix PR middle-end/59049

On Fri, Nov 8, 2013 at 5:02 PM, Jeff Law <law@redhat.com> wrote:
> On 11/08/13 07:45, Steven Bosscher wrote:
>>
>> On Fri, Nov 8, 2013 at 3:40 PM, Joern Rennecke
>> <joern.rennecke@embecosm.com> wrote:
>>>
>>> bootstrapped / regtested on i686-pc-linux-gnu.
>>
>>
>> Not a very elaborate description of the patch, eh? :-)
>>
>> This is IMHO not OK without at least an explanation of why the
>> comparison of two const_ints is not folded. Better yet would be to fix
>> that underlying problem. We should not present such non-sense to the
>> RTL parts of the middle end.
> Agreed.

In this case it's fold-all-builtins folding a strlen call with a
PHI <"foo", "bar"> argument.  IMHO not presenting RTL with such
non-sense is best achieved by not letting TER do constant propagation
(because it doesn't "fold" the result).  We can never rule out such
stray non-propagated constants, so that makes expand more robust
(and hopes for RTL CCP).

   return false;

does that make sense?  I'll test it then.

Thanks,
Richard.

> jeff
>

> In this case it's fold-all-builtins folding a strlen call with a
> PHI <"foo", "bar"> argument.  IMHO not presenting RTL with such
> non-sense is best achieved by not letting TER do constant propagation
> (because it doesn't "fold" the result).  We can never rule out such
> stray non-propagated constants, so that makes expand more robust
> (and hopes for RTL CCP).
> 
> Index: gcc/tree-ssa-ter.c
> ===================================================================
> --- gcc/tree-ssa-ter.c  (revision 204664)
> +++ gcc/tree-ssa-ter.c  (working copy)
> @@ -438,6 +439,12 @@ ter_is_replaceable_p (gimple stmt)
>           && !is_gimple_val (gimple_assign_rhs1 (stmt)))
>         return false;
> 
> +      /* Do not propagate "modeless" constants - we may end up
> confusing the RTL
> +        expanders.  Leave the optimization to RTL CCP.  */
> +      if (gimple_assign_single_p (stmt)
> +         && CONSTANT_CLASS_P (gimple_assign_rhs1 (stmt)))
> +       return false;
> +
>        return true;
>      }
>    return false;
> 
> does that make sense?  I'll test it then.

I agree with Joern that we want more constant propagation/folding during 
RTL expansion, not less, so IMO that's the wrong direction.

On Mon, Nov 11, 2013 at 12:21 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> In this case it's fold-all-builtins folding a strlen call with a
>> PHI <"foo", "bar"> argument.  IMHO not presenting RTL with such
>> non-sense is best achieved by not letting TER do constant propagation
>> (because it doesn't "fold" the result).  We can never rule out such
>> stray non-propagated constants, so that makes expand more robust
>> (and hopes for RTL CCP).
>>
>> Index: gcc/tree-ssa-ter.c
>> ===================================================================
>> --- gcc/tree-ssa-ter.c  (revision 204664)
>> +++ gcc/tree-ssa-ter.c  (working copy)
>> @@ -438,6 +439,12 @@ ter_is_replaceable_p (gimple stmt)
>>           && !is_gimple_val (gimple_assign_rhs1 (stmt)))
>>         return false;
>>
>> +      /* Do not propagate "modeless" constants - we may end up
>> confusing the RTL
>> +        expanders.  Leave the optimization to RTL CCP.  */
>> +      if (gimple_assign_single_p (stmt)
>> +         && CONSTANT_CLASS_P (gimple_assign_rhs1 (stmt)))
>> +       return false;
>> +
>>        return true;
>>      }
>>    return false;
>>
>> does that make sense?  I'll test it then.
>
> I agree with Joern that we want more constant propagation/folding during
> RTL expansion, not less, so IMO that's the wrong direction.

The question is whether you for example want to handle

  a_2 = 1 + 0;

at RTL expansion time?  I'd say it's better to have

  b_1 = 0;
  a_2 = 1 + b_1;

not to say the proposed patch would be a way to ensure the first
didn't happen - it just makes it less likely that TER gets you
into this.

OTOH as TER is now "explicit" (the expander has to lookup SSA defs)
those uses should better deal with constants they receive from that.

Richard.

> --
> Eric Botcazou

On Mon, Nov 11, 2013 at 12:26:09PM +0100, Richard Biener wrote:
> The question is whether you for example want to handle
> 
>   a_2 = 1 + 0;
> 
> at RTL expansion time?  I'd say it's better to have

I think we already handle that just fine, there are tons of various
simplify_gen_* calls during expansion, and we know there the mode etc.

Just Joern hit a place which wasn't prepared to handle it properly,
so either we handle it as you are suggesting by forcing one of the
constants into a register, or we simplify the comparison and if it
simplifies into a constant, we transform it to something simpler.

	Jakub