[PATCH] Fix PR55876 - Make generation of paradoxical subreg in widen_operand more robust

[PATCH] Fix PR55876 - Make generation of paradoxical subreg in widen_operand more robust

[Tom de Vries]

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

[with CC to gcc-patches]

-------- Original Message --------
Subject: [PATCH] Fix PR55876 - Make generation of paradoxical subreg in
widen_operand more robust
Date: Mon, 07 Jan 2013 11:13:49 +0100
From: Tom de Vries <Tom_deVries@mentor.com>
To: Richard Henderson <rth@redhat.com>
CC: Steve Ellcey <sellcey@mips.com>, Andrew Pinski <pinskia@gmail.com>

Richard,

Consider test-case test.c:
...
static inline unsigned char
bar (const char *b)
{
   unsigned char used = 0;
   int i;

   for (i = 0; i < 4; ++i)
     if (b[i] != 'F')
       used = 1;

   return used;
}

static char buffer[8];

unsigned char
foo (void)
{
  return bar (buffer) ? 0 : 1;
}
...

When compiling test.c with a mips compiler, this ICE triggers:
...
$ ./install/bin/mips-linux-gnu-gcc -O3 test.c -S -mabi=64 -march=mips64
test.c: In function 'foo':
test.c:19:3: internal compiler error: in gen_rtx_SUBREG, at emit-rtl.c:776
   return bar (buffer) ? 0 : 1;
...

The ICE is introduced by revision r193539 discussed at
http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01148.html .


The representation of foo just before expand is this:
...
foo ()
{
  unsigned charD.13 usedD.1407;
  charD.2 _7;
  unsigned charD.13 _13;
  charD.2 _19;
  charD.2 _28;
  charD.2 _37;

;;   basic block 2, loop depth 0
;;    pred:       ENTRY
  # VUSE <.MEM_1(D)>
  _19 = MEM[(const charD.2 *)&bufferD.1387];
  used_20 = _19 != 70 ? 1 : 0;
  # VUSE <.MEM_1(D)>
  _28 = MEM[(const charD.2 *)&bufferD.1387 + 1B];
  used_29 = _28 == 70 ? used_20 : 1;
  # VUSE <.MEM_1(D)>
  _37 = MEM[(const charD.2 *)&bufferD.1387 + 2B];
  used_38 = _37 == 70 ? used_29 : 1;
  # VUSE <.MEM_1(D)>
  _7 = MEM[(const charD.2 *)&bufferD.1387 + 3B];
  used_10 = _7 == 70 ? used_38 : 1;
  _13 = used_10 ^ 1;
  # VUSE <.MEM_1(D)>
  return _13;
;;    succ:       EXIT

}
...

The used_10 operand is in a DImode reg because r193539 allows it to be promoted
while expanding
  used_10 = _7 == 70 ? used_38 : 1
in expand_cond_expr_using_cmove.

The ICE happens during expansion of
  _13 = used_10 ^ 1
when trying to widen the DIMode reg for use_10 from QImode to SImode:
...
#6  0x085d7da5 in widen_operand (op=0xf7cd2ec0, mode=SImode, oldmode=QImode,
unsignedp=1, no_extend=1) at
/home/vries/local/mips/upstream/src/gcc-mainline/gcc/optabs.c:333
333         return gen_rtx_SUBREG (mode, force_reg (GET_MODE (op), op), 0);
(gdb) call debug_rtx (op)
(reg:DI 222 [ used+-7 ])
...

And although the comment in widen_operand states that we're generating a
paradoxical subreg:
...
  /* If MODE is no wider than a single word, we return a paradoxical
     SUBREG.  */
  if (GET_MODE_SIZE (mode) <= UNITS_PER_WORD)
    return gen_rtx_SUBREG (mode, force_reg (GET_MODE (op), op), 0);
...
it's not because mode == SImode and GET_MODE (op) == DImode.

Then validate_subreg triggers the ICE in gen_rtx_SUBREG by returning false here:
...
  /* For pseudo registers, we want most of the same checks.  Namely:
     If the register no larger than a word, the subreg must be lowpart.
     If the register is larger than a word, the subreg must be the lowpart
     of a subword.  A subreg does *not* perform arbitrary bit extraction.
     Given that we've already checked mode/offset alignment, we only have
     to check subword subregs here.  */
  if (osize < UNITS_PER_WORD
      && ! (lra_in_progress && (FLOAT_MODE_P (imode) || FLOAT_MODE_P (omode))))
    {
      enum machine_mode wmode = isize > UNITS_PER_WORD ? word_mode : imode;
      unsigned int low_off = subreg_lowpart_offset (omode, wmode);
      if (offset % UNITS_PER_WORD != low_off)
	return false;
    }
...
For a valid pseudo subreg with outer mode SImode and inner mode DImode we need
the offset corresponding to the lowpart, which is 4 for -EB. But since we were
trying to generate a paradoxical subreg, the offset is 0. This explains why the
assert doesn't trigger with -EL.

Attached patch (build and tested for target mips-linux-gnu) prevents the ICE by
checking in widen_operand whether the result of the gen_rtx_SUBREG call would
indeed be a paradoxical subreg. As a consequence, it handles this case now here:
...
  /* Otherwise, get an object of MODE, clobber it, and set the low-order
     part to OP.  */

  result = gen_reg_rtx (mode);
  emit_clobber (result);
  emit_move_insn (gen_lowpart (GET_MODE (op), result), op);
  return result;
...

So the generated code is this:
...
(insn 34 33 35 2 (clobber (reg:SI 228)) -1
     (nil))
(insn 35 34 36 2 (set (subreg:DI (reg:SI 228) 0)
        (reg:DI 222 [ usedD.1407+-7 ])) -1
     (nil))
(insn 36 35 37 2 (set (reg:SI 229)
        (xor:SI (reg:SI 228)
            (const_int 1 [0x1]))) -1
...
which is correct.


I've just realized that this is probably a too conservative fix. Using this patch:
...
Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c (revision 194898)
+++ gcc/optabs.c (working copy)
@@ -327,10 +327,15 @@ widen_operand (rtx op, enum machine_mode
          && SUBREG_PROMOTED_UNSIGNED_P (op) == unsignedp))
     return convert_modes (mode, oldmode, op, unsignedp);

-  /* If MODE is no wider than a single word, we return a paradoxical
-     SUBREG.  */
+  /* If MODE is no wider than a single word, we return a
+     lowpart or paradoxical SUBREG.  */
   if (GET_MODE_SIZE (mode) <= UNITS_PER_WORD)
-    return gen_rtx_SUBREG (mode, force_reg (GET_MODE (op), op), 0);
+    {
+      if (GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (op)))
+       return gen_lowpart_SUBREG (mode, op);
+      else
+       return gen_rtx_SUBREG (mode, force_reg (GET_MODE (op), op), 0);
+    }

   /* Otherwise, get an object of MODE, clobber it, and set the low-order
      part to OP.  */
...

we generate this instead:
...
(insn 34 33 35 2 (set (reg:SI 228)
        (xor:SI (subreg:SI (reg:DI 222 [ usedD.1407+-7 ]) 4)
            (const_int 1 [0x1]))) -1
...
which is similar to what we generate with trunk for -EL.


So, is the attached patch ok for trunk?

Or should I test the more optimal patch listed above?

Or should this be fixed somewhere else?

Thanks,
- Tom

2013-01-07  Tom de Vries  <tom@codesourcery.com>

	PR target/55876
	* optabs.c (optabs.c): Add condition to ensure that subreg will be
	paradoxical.




[-- Attachment #2: widen_operand-paradoxical-subreg.patch --]
[-- Type: text/x-patch, Size: 609 bytes --]

Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c (revision 194898)
+++ gcc/optabs.c (working copy)
@@ -329,7 +329,8 @@ widen_operand (rtx op, enum machine_mode
 
   /* If MODE is no wider than a single word, we return a paradoxical
      SUBREG.  */
-  if (GET_MODE_SIZE (mode) <= UNITS_PER_WORD)
+  if (GET_MODE_SIZE (mode) <= UNITS_PER_WORD
+      && GET_MODE_SIZE (mode) > GET_MODE_SIZE (GET_MODE (op)))
     return gen_rtx_SUBREG (mode, force_reg (GET_MODE (op), op), 0);
 
   /* Otherwise, get an object of MODE, clobber it, and set the low-order

Re: [PATCH] Fix PR55876 - Make generation of paradoxical subreg in widen_operand more robust

[Richard Henderson]

On 01/07/2013 02:16 AM, Tom de Vries wrote:
> -  /* If MODE is no wider than a single word, we return a paradoxical
> -     SUBREG.  */
> +  /* If MODE is no wider than a single word, we return a
> +     lowpart or paradoxical SUBREG.  */
>    if (GET_MODE_SIZE (mode) <= UNITS_PER_WORD)
> -    return gen_rtx_SUBREG (mode, force_reg (GET_MODE (op), op), 0);
> +    {
> +      if (GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (op)))
> +       return gen_lowpart_SUBREG (mode, op);
> +      else
> +       return gen_rtx_SUBREG (mode, force_reg (GET_MODE (op), op), 0);
> +    }

Is there any good reason we're using gen_rtx_SUBREG directly here?
Seems like this sort of logic would be present in plain gen_lowpart,
generating the paradoxical subreg that we want.


r~

Re: [PATCH] Fix PR55876 - Make generation of paradoxical subreg in widen_operand more robust

[Tom de Vries]

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

On 07/01/13 18:48, Richard Henderson wrote:
> On 01/07/2013 02:16 AM, Tom de Vries wrote:
>> -  /* If MODE is no wider than a single word, we return a paradoxical
>> -     SUBREG.  */
>> +  /* If MODE is no wider than a single word, we return a
>> +     lowpart or paradoxical SUBREG.  */
>>    if (GET_MODE_SIZE (mode) <= UNITS_PER_WORD)
>> -    return gen_rtx_SUBREG (mode, force_reg (GET_MODE (op), op), 0);
>> +    {
>> +      if (GET_MODE_SIZE (mode) < GET_MODE_SIZE (GET_MODE (op)))
>> +       return gen_lowpart_SUBREG (mode, op);
>> +      else
>> +       return gen_rtx_SUBREG (mode, force_reg (GET_MODE (op), op), 0);
>> +    }
> 
> Is there any good reason we're using gen_rtx_SUBREG directly here?
> Seems like this sort of logic would be present in plain gen_lowpart,
> generating the paradoxical subreg that we want.
> 

Ah, didn't realize that. Attached patch uses gen_lowpart.

Bootstrapped and reg-tested on x86_64.

Build and reg-tested on mips64, -mabi=n32 and -mabi=64.

OK for trunk?

Thanks,
- Tom

2013-01-14  Tom de Vries  <tom@codesourcery.com>

	PR target/55876
	* optabs.c (widen_operand): Use gen_lowpart instead of gen_rtx_SUBREG.
	Update comment.


[-- Attachment #2: widen_operand-paradoxical-subreg.4.patch --]
[-- Type: text/x-patch, Size: 778 bytes --]

Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c (revision 194898)
+++ gcc/optabs.c (working copy)
@@ -327,10 +327,10 @@ widen_operand (rtx op, enum machine_mode
 	  && SUBREG_PROMOTED_UNSIGNED_P (op) == unsignedp))
     return convert_modes (mode, oldmode, op, unsignedp);
 
-  /* If MODE is no wider than a single word, we return a paradoxical
+  /* If MODE is no wider than a single word, we return a lowpart or paradoxical
      SUBREG.  */
   if (GET_MODE_SIZE (mode) <= UNITS_PER_WORD)
-    return gen_rtx_SUBREG (mode, force_reg (GET_MODE (op), op), 0);
+    return gen_lowpart (mode, force_reg (GET_MODE (op), op));
 
   /* Otherwise, get an object of MODE, clobber it, and set the low-order
      part to OP.  */

Re: [PATCH] Fix PR55876 - Make generation of paradoxical subreg in widen_operand more robust

[Richard Henderson]

On 01/14/2013 12:27 AM, Tom de Vries wrote:
> 2013-01-14  Tom de Vries<tom@codesourcery.com>
>
> 	PR target/55876
> 	* optabs.c (widen_operand): Use gen_lowpart instead of gen_rtx_SUBREG.
> 	Update comment.

Ok.


r~