RFA: Fix PR middle-end/59049

Submitted by Joern Rennecke on Nov. 8, 2013, 2:40 p.m.

Details

Message ID CAMqJFCqWDNTjmJ11p-LGE4vBPQ7gBSLWY3prXFNv7+QUoL_ipw@mail.gmail.com
State New
Headers show

Commit Message

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.

Comments

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 hide | download patch | download mbox

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