RFA: Fix PR middle-end/59049

RFA: Fix PR middle-end/59049

[Joern Rennecke]

[-- Attachment #1: Type: text/plain, Size: 47 bytes --]

bootstrapped / regtested on i686-pc-linux-gnu.

[-- Attachment #2: .txt --]
[-- Type: text/plain, Size: 909 bytes --]

2013-11-08  Joern Rennecke  <joern.rennecke@embecosm.com>

	PR middle-end/59049
	* expmed.c (emit_cstore): Avoid generating a comparison of two
	VOIDmode constants.

Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c	(revision 204568)
+++ gcc/expmed.c	(working copy)
@@ -5112,6 +5112,12 @@ emit_cstore (rtx target, enum insn_code
   if (!target)
     target = gen_reg_rtx (target_mode);
 
+  /* If we compare two VOIDmode constants, we loose the mode to do the
+     comparison in.  To avoid that, copy one constant into a register.
+     We rely on subsequent optimizations (like ce1) to clean this up.  */
+  if (GET_MODE (x) == VOIDmode && GET_MODE (y) == VOIDmode)
+    x = copy_to_mode_reg (compare_mode, x);
+
   comparison = gen_rtx_fmt_ee (code, result_mode, x, y);
 
   create_output_operand (&ops[0], optimize ? NULL_RTX : target, result_mode);

Re: RFA: Fix PR middle-end/59049

[Steven Bosscher]

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.

Also would need a test case.

Ciao!
Steven

Re: RFA: Fix PR middle-end/59049

[Joern Rennecke]

On 8 November 2013 14:45, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> Also would need a test case.

As is mentioned in the PR, we already have a test case, which shows a
regression.

Re: RFA: Fix PR middle-end/59049

[Joern Rennecke]

On 8 November 2013 14:45, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> 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.

Which part would be responsible to folding the comparison at -O1 ?
FWIW, as I just commented in the PR, one of the operands is an ssa_name
with a known value.

Re: RFA: Fix PR middle-end/59049

[Steven Bosscher]

On Fri, Nov 8, 2013 at 4:18 PM, Joern Rennecke wrote:
> On 8 November 2013 14:45, Steven Bosscher wrote:
>> 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.
>
> Which part would be responsible to folding the comparison at -O1 ?
> FWIW, as I just commented in the PR, one of the operands is an ssa_name
> with a known value.

This usually is taken care of by one of the many propagation passes
(CCP, VRP, forwprop, DOM, even FRE/PRE). The only cases I've
previously encountered, where a known value ends up not being
propagated, is when the propagation is somehow overlooked and the
known value is only replaced at out-of-ssa time, i.e. TER.

The only way to find out where the propagation should have happened,
is by looking at the gimple dumps.

Ciao!
Steven

Re: RFA: Fix PR middle-end/59049

[Jakub Jelinek]

On Fri, Nov 08, 2013 at 04:32:09PM +0100, Steven Bosscher wrote:
> On Fri, Nov 8, 2013 at 4:18 PM, Joern Rennecke wrote:
> > On 8 November 2013 14:45, Steven Bosscher wrote:
> >> 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.
> >
> > Which part would be responsible to folding the comparison at -O1 ?
> > FWIW, as I just commented in the PR, one of the operands is an ssa_name
> > with a known value.
> 
> This usually is taken care of by one of the many propagation passes
> (CCP, VRP, forwprop, DOM, even FRE/PRE). The only cases I've
> previously encountered, where a known value ends up not being
> propagated, is when the propagation is somehow overlooked and the
> known value is only replaced at out-of-ssa time, i.e. TER.
> 
> The only way to find out where the propagation should have happened,
> is by looking at the gimple dumps.

Well, most of the passes you've mentioned can be disabled with -fno-tree-*
but still we shouldn't ICE on it, so the expander can't assume that
it will never see something like that, only doesn't have to try too hard
to optimize that unlikely case.  Of course, it is nice to know why it hasn't
been optimized in the particular case.

	Jakub

Re: RFA: Fix PR middle-end/59049

[Steven Bosscher]

On Fri, Nov 8, 2013 at 4:41 PM, Jakub Jelinek wrote:
> On Fri, Nov 08, 2013 at 04:32:09PM +0100, Steven Bosscher wrote:
>> On Fri, Nov 8, 2013 at 4:18 PM, Joern Rennecke wrote:
>> > On 8 November 2013 14:45, Steven Bosscher wrote:
>> >> 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.
>> >
>> > Which part would be responsible to folding the comparison at -O1 ?
>> > FWIW, as I just commented in the PR, one of the operands is an ssa_name
>> > with a known value.
>>
>> This usually is taken care of by one of the many propagation passes
>> (CCP, VRP, forwprop, DOM, even FRE/PRE). The only cases I've
>> previously encountered, where a known value ends up not being
>> propagated, is when the propagation is somehow overlooked and the
>> known value is only replaced at out-of-ssa time, i.e. TER.
>>
>> The only way to find out where the propagation should have happened,
>> is by looking at the gimple dumps.
>
> Well, most of the passes you've mentioned can be disabled with -fno-tree-*
> but still we shouldn't ICE on it, so the expander can't assume that
> it will never see something like that, only doesn't have to try too hard
> to optimize that unlikely case.  Of course, it is nice to know why it hasn't
> been optimized in the particular case.


Even with the gimple opts disabled, a const-const comparison would
normally be folded by the RTL expanders.

Ciao!
Steven

Re: RFA: Fix PR middle-end/59049

[Jeff Law]

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.
jeff

Re: RFA: Fix PR middle-end/59049

[Joern Rennecke]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

On 8 November 2013 15:50, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> Even with the gimple opts disabled, a const-const comparison would
> normally be folded by the RTL expanders.

Well, in this spirit, attached is another way to address the RTL side
of the problem.
As mention in the PR, the tree side of the problem started showing up
in r204194.

[-- Attachment #2: .txt --]
[-- Type: text/plain, Size: 864 bytes --]

2013-11-08  Joern Rennecke  <joern.rennecke@embecosm.com>

	PR middle-end/59049
	* expmed.c (emit_store_flag): Fail for const-const comparison.

Index: expmed.c
===================================================================
--- expmed.c	(revision 204568)
+++ expmed.c	(working copy)
@@ -5401,6 +5401,13 @@ emit_store_flag (rtx target, enum rtx_co
   rtx subtarget;
   rtx tem, last, trueval;
 
+  /* If we compare constants, we shouldn't use a store-flag operation,
+     but a constant load.  We can get there via the vanilla route that
+     usually generates a compare-branch sequence, but will in this case
+     fold the comparison to a constant, and thus elide the branch.  */
+  if (CONSTANT_P (op0) && CONSTANT_P (op1))
+    return 0;
+
   tem = emit_store_flag_1 (target, code, op0, op1, mode, unsignedp, normalizep,
 			   target_mode);
   if (tem)

Re: RFA: Fix PR middle-end/59049

[Richard Biener]

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).

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.

Thanks,
Richard.

> jeff
>

Re: RFA: Fix PR middle-end/59049

[Eric Botcazou]

> 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.

-- 
Eric Botcazou

Re: RFA: Fix PR middle-end/59049

[Richard Biener]

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

Re: RFA: Fix PR middle-end/59049

[Jakub Jelinek]

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

Re: RFA: Fix PR middle-end/59049

[Jakub Jelinek]

On Fri, Nov 08, 2013 at 05:11:39PM +0000, Joern Rennecke wrote:
> On 8 November 2013 15:50, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> > Even with the gimple opts disabled, a const-const comparison would
> > normally be folded by the RTL expanders.
> 
> Well, in this spirit, attached is another way to address the RTL side
> of the problem.
> As mention in the PR, the tree side of the problem started showing up
> in r204194.

> 2013-11-08  Joern Rennecke  <joern.rennecke@embecosm.com>
> 
> 	PR middle-end/59049
> 	* expmed.c (emit_store_flag): Fail for const-const comparison.

LGTM (I'd just write return NULL_RTX; instead of return 0;).

> --- expmed.c	(revision 204568)
> +++ expmed.c	(working copy)
> @@ -5401,6 +5401,13 @@ emit_store_flag (rtx target, enum rtx_co
>    rtx subtarget;
>    rtx tem, last, trueval;
>  
> +  /* If we compare constants, we shouldn't use a store-flag operation,
> +     but a constant load.  We can get there via the vanilla route that
> +     usually generates a compare-branch sequence, but will in this case
> +     fold the comparison to a constant, and thus elide the branch.  */
> +  if (CONSTANT_P (op0) && CONSTANT_P (op1))
> +    return 0;
> +
>    tem = emit_store_flag_1 (target, code, op0, op1, mode, unsignedp, normalizep,
>  			   target_mode);
>    if (tem)


	Jakub