Patchwork RFA: Fix PR middle-end/59049

login
register
mail settings
Submitter Joern Rennecke
Date Nov. 8, 2013, 2:40 p.m.
Message ID <CAMqJFCqWDNTjmJ11p-LGE4vBPQ7gBSLWY3prXFNv7+QUoL_ipw@mail.gmail.com>
Download mbox | patch
Permalink /patch/289853/
State New
Headers show

Comments

Joern Rennecke - Nov. 8, 2013, 2:40 p.m.
bootstrapped / regtested on i686-pc-linux-gnu.
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.
Steven Bosscher - Nov. 8, 2013, 2:45 p.m.
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
Joern Rennecke - Nov. 8, 2013, 2:58 p.m.
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.
Joern Rennecke - Nov. 8, 2013, 3:18 p.m.
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.
Steven Bosscher - Nov. 8, 2013, 3:32 p.m.
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
Jakub Jelinek - Nov. 8, 2013, 3:41 p.m.
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
Steven Bosscher - Nov. 8, 2013, 3:50 p.m.
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
Jeff Law - Nov. 8, 2013, 4:02 p.m.
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

Patch

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