diff mbox

[middle-end] : Fix also PR46292 (was: Fix PR 44569)

Message ID AANLkTikhFGbPbtsCRCxqTHqkU7Wjgpr_MB4QRfjrho=o@mail.gmail.com
State New
Headers show

Commit Message

Uros Bizjak Nov. 3, 2010, 8:02 p.m. UTC 
On Tue, Nov 2, 2010 at 7:31 PM, Paolo Bonzini <bonzini@gnu.org> wrote:

>> >  Out of interest, is this canonical rtl?  I wasn't sure whether it
>> >  should be a const_vector instead.
>>
>> We probably can end up with various non-canonical RTL in debug-insns
>> (like we can end up with non-gimple trees in debug statements).  Sad
>> but true.
>
> If the choice is between a CONST_VECTOR and a CONCATN, a precise definition
> of the one that is incorrect (I don't know which) would be "wrong RTL"
> rather than "non-canonical RTL". :)

Apparently, some targets return CONCATN in TImode:

(concatn:TI [
        (const_int 1 [0x1])
        (const_int 2 [0x2])
        (const_int 3 [0x3])
        (const_int 4 [0x4])
        (const_int 1 [0x1])
        (const_int 2 [0x2])
        (const_int 3 [0x3])
        (const_int 4 [0x4])
        (const_int 1 [0x1])
        (const_int 2 [0x2])
        (const_int 3 [0x3])
        (const_int 4 [0x4])
        (const_int 1 [0x1])
        (const_int 2 [0x2])
        (const_int 3 [0x3])
        (const_int 4 [0x4])
    ])

This RTX breaks GET_MODE_INNER, but we can win the game with another
trick in the sleeve:

2010-11-03  Uros Bizjak  <ubizjak@gmail.com>

	PR middle-end/46292
	* lower-suberg.c (simplify_subreg_concatn): For VOIDmode elements,
	if the innermode is not vector mode, determine the mode of a subreg
	by using mode_for_size of inner_size.

The patch was bootstrapped and regression tested on
x86_64-pc-linux-gnu {,-m32}, it was also tested with the cross to
hppa-unknown-linux-gnu (where it fixes ICE).

OK for 4.3 - 4.6 ?

Uros.

Comments

Eric Botcazou Nov. 3, 2010, 9:38 p.m. UTC | #1 
> This RTX breaks GET_MODE_INNER, but we can win the game with another
> trick in the sleeve:

Playing tricks on the 4.3/4.4 branches doesn't seem very appropriate to me at 
this stage.  PR middle-end/44569 wasn't a regression so I wonder why the 
patch was even installed on the 4.5 branch; and there is no testcase AFAICS.
Uros Bizjak Nov. 3, 2010, 9:46 p.m. UTC | #2 
On Wed, Nov 3, 2010 at 10:38 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> This RTX breaks GET_MODE_INNER, but we can win the game with another
>> trick in the sleeve:
>
> Playing tricks on the 4.3/4.4 branches doesn't seem very appropriate to me at
> this stage.  PR middle-end/44569 wasn't a regression so I wonder why the
> patch was even installed on the 4.5 branch; and there is no testcase AFAICS.

It was "obvious" in the sense that avoided ICE by correcting VOIDmode
for vector mode operands. So, it was either ICE or to dig a little
deeper in the operand itself for a correct mode. Due to this logic,
the patch can't cause any regression.

Uros.
Eric Botcazou Nov. 3, 2010, 10:13 p.m. UTC | #3 
> It was "obvious" in the sense that avoided ICE by correcting VOIDmode
> for vector mode operands. So, it was either ICE or to dig a little
> deeper in the operand itself for a correct mode. Due to this logic,
> the patch can't cause any regression.

But that's backward, the patch must fix a regression to be put on branches; at 
a minimum, there must be a testcase to show that there is a real problem to 
be fixed on the branches.  There is neither in this case AFAICS.
Uros Bizjak Nov. 3, 2010, 10:22 p.m. UTC | #4 
On Wed, Nov 3, 2010 at 11:13 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> It was "obvious" in the sense that avoided ICE by correcting VOIDmode
>> for vector mode operands. So, it was either ICE or to dig a little
>> deeper in the operand itself for a correct mode. Due to this logic,
>> the patch can't cause any regression.
>
> But that's backward, the patch must fix a regression to be put on branches; at
> a minimum, there must be a testcase to show that there is a real problem to
> be fixed on the branches.  There is neither in this case AFAICS.

OK, I can revert the patch on branches.

Uros.
Eric Botcazou Nov. 4, 2010, 2:49 p.m. UTC | #5 
> OK, I can revert the patch on branches.

Thanks.

The patch is OK for mainline if you add a comment explaining where these 2 
kinds of CONCATN come from, e.g.:

  /* VECTOR_CSTs in debug expressions are expanded into CONCATN instead of
     regular CONST_VECTORs.  They have vector or integer modes, depending
     on the capabilities of the target.  Cope with them.  */
  if (partmode == VOIDmode && VECTOR_MODE_P (innermode))
    partmode = GET_MODE_INNER (innermode);
  else if (partmode == VOIDmode)
    {
      enum mode_class mclass = GET_MODE_CLASS (innermode);
      partmode = mode_for_size (inner_size * BITS_PER_UNIT, mclass, 0);
    }
diff mbox

Patch

Index: lower-subreg.c
===================================================================
--- lower-subreg.c	(revision 166266)
+++ lower-subreg.c	(working copy)
@@ -411,10 +411,13 @@  simplify_subreg_concatn (enum machine_mo
   part = XVECEXP (op, 0, byte / inner_size);
   partmode = GET_MODE (part);
 
+  if (partmode == VOIDmode && VECTOR_MODE_P (innermode))
+    partmode = GET_MODE_INNER (innermode);
+
   if (partmode == VOIDmode)
     {
-      gcc_assert (VECTOR_MODE_P (innermode));
-      partmode = GET_MODE_INNER (innermode);
+      enum mode_class mclass = GET_MODE_CLASS (innermode);
+      partmode = mode_for_size (inner_size * BITS_PER_UNIT, mclass, 0);
     }
 
   final_offset = byte % inner_size;