messy combine problem - PPC

messy combine problem - PPC

[Zack Weinberg]

I'm looking at a nasty combine problem exposed by my PPC ABI patches.
Suppose you have code like this:

struct a { 
  char hours, day, month;
  short year;
};

extern struct a A;
extern struct a ret_struct_a();

void test(void)
{
  A = ret_struct_a();
}

Right before combine, we have this RTL:

(call_insn 8 48 9 (parallel[ 
            (set (reg:DI 3 r3)
                (call (mem:SI (symbol_ref:SI ("ret_struct_a")) [0 S4 A32])
                    (const_int 0 [0x0])))
            (use (const_int 0 [0x0]))
            (clobber (scratch:SI))
        ] ) -1 (nil)
    (expr_list:REG_UNUSED (scratch:SI)
        (nil))
    (nil))

(insn 9 8 11 (set (reg:SI 115)
        (zero_extend:SI (subreg:HI (reg:SI 3 r3) 2)))
	 30 {*rs6000.md:1385}
    (insn_list 8 (nil))
    (expr_list:REG_DEAD (reg:SI 3 r3)
        (nil)))

As far as I know this is correct RTL.  Combine tries to do something
(I'm not entirely sure what) with insn 9.  At -O1, in the course of
that it calls get_last_value for

  (subreg:HI (reg:SI 3 r3) 2)

and get_last_value obligingly tries to return the call pattern wrapped
in a SUBREG.  This blows up in simplify_gen_subreg, since the call RTX
has no mode.

Breakpoint 1, fancy_abort (
    file=0x8439ba0 "../../../gcc/ppc-eabi/gcc/simplify-rtx.c", line=2649, 
    function=0x843a11b "simplify_gen_subreg")
    at ../../../gcc/ppc-eabi/gcc/diagnostic.c:1450
1450      internal_error ("Internal compiler error in %s, at %s:%d",
(gdb) bt
#0  fancy_abort (file=0x8439ba0 "../../../gcc/ppc-eabi/gcc/simplify-rtx.c", 
    line=2649, function=0x843a11b "simplify_gen_subreg")
    at ../../../gcc/ppc-eabi/gcc/diagnostic.c:1450
#1  0x082b61a9 in simplify_gen_subreg (outermode=HImode, op=0x40174df8, 
    innermode=VOIDmode, byte=0)
    at ../../../gcc/ppc-eabi/gcc/simplify-rtx.c:2649
#2  0x0834d2aa in gen_lowpart_for_combine (mode=HImode, x=0x40174df8)
    at ../../../gcc/ppc-eabi/gcc/combine.c:9748
#3  0x08351cc8 in get_last_value (x=0x4016ea20)
    at ../../../gcc/ppc-eabi/gcc/combine.c:11366


At -O2, -fexpensive-optimizations is on, and 

  (zero_extend:SI (subreg:HI (reg:SI 3 r3) 2))

is replaced by

  (and:SI (reg:SI 3 r3) (const_int 65535 [0xffff]))

before anything else happens; this goes through a different code path,
combine asks for the last value of (reg:SI 3 r3), and
simplify_gen_subreg is not asked to do anything with the CALL pattern.

The $64,000 question: what's the right fix here?  The incoming RTL is
correct, so a fix in the machine description is inappropriate.
simplify_gen_subreg is correct to abort, (subreg:HI (call ...) N) is
ill-formed.  I'm not familiar enough with combine to know where in
between these two the fix should go.

Ideas?

zw

Re: messy combine problem - PPC

[Richard Henderson]

On Mon, Dec 03, 2001 at 12:49:34PM -0800, Zack Weinberg wrote:
> Ideas?

Make record_dead_and_set_regs return after processing a CALL_INSN,
rather than recording that r3 is set from a call pattern.

Also, that should really be using regs_invalidated_by_call rather
than call_used_regs.  Not that it'll make too much difference, 
since we don't do a whole lot of combining with fixed registers.


r~

Re: messy combine problem - PPC

[Zack Weinberg]

On Mon, Dec 03, 2001 at 03:06:06PM -0800, Richard Henderson wrote:
> On Mon, Dec 03, 2001 at 12:49:34PM -0800, Zack Weinberg wrote:
> > Ideas?
> 
> Make record_dead_and_set_regs return after processing a CALL_INSN,
> rather than recording that r3 is set from a call pattern.

I'll try this, but will it inhibit desirable optimizations?

> Also, that should really be using regs_invalidated_by_call rather
> than call_used_regs.  Not that it'll make too much difference, 
> since we don't do a whole lot of combining with fixed registers.

Huh?

zw

Re: messy combine problem - PPC

[Richard Henderson]

On Mon, Dec 03, 2001 at 03:53:12PM -0800, Zack Weinberg wrote:
> I'll try this, but will it inhibit desirable optimizations?

No.

> > Also, that should really be using regs_invalidated_by_call rather
> > than call_used_regs.  Not that it'll make too much difference, 
> > since we don't do a whole lot of combining with fixed registers.
> 
> Huh?

       for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
-        if (call_used_regs[i])
+	 if (TEST_HARD_REG_BIT (regs_invalidated_by_call, i))



r~

Re: messy combine problem - PPC

[Zack Weinberg]

On Mon, Dec 03, 2001 at 06:27:06PM -0800, Richard Henderson wrote:
> On Mon, Dec 03, 2001 at 03:53:12PM -0800, Zack Weinberg wrote:
> > I'll try this, but will it inhibit desirable optimizations?
> 
> No.

I thought about it a bit more, and it can't ever inhibit an optimization,
but I am still worried about not recording registers that die in call
instructions.  Consider

  result = (*fptr) (args...);

where fptr is not used after that, and combine will see RTL such as 

  (set (reg:SI result)
       (call (reg:P fptr)))

or is there something else that takes care of that?

> > > Also, that should really be using regs_invalidated_by_call rather
> > > than call_used_regs.  Not that it'll make too much difference, 
> > > since we don't do a whole lot of combining with fixed registers.
> > 
> > Huh?
> 
>        for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
> -        if (call_used_regs[i])
> +	 if (TEST_HARD_REG_BIT (regs_invalidated_by_call, i))

Oh, you meant the loop in record_dead_and_set_regs.

zw

Re: messy combine problem - PPC

[Richard Henderson]

On Mon, Dec 03, 2001 at 10:34:31PM -0800, Zack Weinberg wrote:
> or is there something else that takes care of that?

Yes, that call_used_regs killing loop in record_dead_and_set_regs.


r~

Re: messy combine problem - PPC

[Zack Weinberg]

On Tue, Dec 04, 2001 at 09:30:58AM -0800, Richard Henderson wrote:
> On Mon, Dec 03, 2001 at 10:34:31PM -0800, Zack Weinberg wrote:
> > or is there something else that takes care of that?
> 
> Yes, that call_used_regs killing loop in record_dead_and_set_regs.

I get it now, thanks for clarifying.  I'm testing this patch.

zw

	* combine.c (record_dead_and_set_regs): Use regs_invalidated_by_call.
	Do not call note_stores for CALL_INSNs.

===================================================================
Index: combine.c
--- combine.c	2001/11/11 11:25:14	1.241
+++ combine.c	2001/12/04 18:05:50
@@ -11178,7 +11178,7 @@ record_dead_and_set_regs (insn)
   if (GET_CODE (insn) == CALL_INSN)
     {
       for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
-	if (call_used_regs[i])
+	if (TEST_HARD_REG_BIT (regs_invalidated_by_call, i))
 	  {
 	    reg_last_set_value[i] = 0;
 	    reg_last_set_mode[i] = 0;
@@ -11188,6 +11188,13 @@ record_dead_and_set_regs (insn)
 	  }
 
       last_call_cuid = mem_last_set = INSN_CUID (insn);
+
+      /* Don't bother recording what this insn does.  It might set the
+	 return value register, but we can't combine into a call
+	 pattern anyway, so there's no point trying (and it may cause
+	 a crash, if e.g. we wind up asking for last_set_value of a
+	 SUBREG of the return value register).  */
+      return;
     }
 
   note_stores (PATTERN (insn), record_dead_and_set_regs_1, insn);