store_motion query

store_motion query

[DJ Delorie]

Why is store_motion doing this?

STORE_MOTION  delete insn in BB 2:
      (insn 8 7 27 2 dj.c:10 (parallel [
                        (set (mem/s/j:HI (reg/v/f:HI 26 [ s ]) [0 <variable>.buf+0 S2 A8])
                            (ashift:HI (reg/v:HI 27 [ n ])
                                (subreg:QI (reg/v:HI 27 [ n ]) 0)))
                        (clobber (scratch:HI))
                    ]) 223 {ashlhi3_i} (expr_list:REG_DEAD (reg/v:HI 27 [ n ])
                    (nil)))
STORE MOTION  replaced with insn:
      (insn 27 8 9 2 dj.c:10 (set (reg:HI 29 [ <variable>.buf ])
                    (ashift:HI (reg/v:HI 27 [ n ])
                        (subreg:QI (reg/v:HI 27 [ n ]) 0))) -1 (expr_list:REG_DEAD (reg/v:HI 27 [ n ])
                    (nil)))
deleting insn with uid = 8.


Notice that the clobber is gone.  Sure enough, later on recog fails
because the clobber is missing.

(this is gcc.dg/torture/pr24257.c at -O1 for m32c-elf)

Re: store_motion query

[Daniel Berlin]

On Wed, May 21, 2008 at 4:48 PM, DJ Delorie <dj@redhat.com> wrote:
>
> Why is store_motion doing this?
>
> STORE_MOTION  delete insn in BB 2:
>      (insn 8 7 27 2 dj.c:10 (parallel [
>                        (set (mem/s/j:HI (reg/v/f:HI 26 [ s ]) [0 <variable>.buf+0 S2 A8])
>                            (ashift:HI (reg/v:HI 27 [ n ])
>                                (subreg:QI (reg/v:HI 27 [ n ]) 0)))
>                        (clobber (scratch:HI))
>                    ]) 223 {ashlhi3_i} (expr_list:REG_DEAD (reg/v:HI 27 [ n ])
>                    (nil)))
> STORE MOTION  replaced with insn:
>      (insn 27 8 9 2 dj.c:10 (set (reg:HI 29 [ <variable>.buf ])
>                    (ashift:HI (reg/v:HI 27 [ n ])
>                        (subreg:QI (reg/v:HI 27 [ n ]) 0))) -1 (expr_list:REG_DEAD (reg/v:HI 27 [ n ])
>                    (nil)))
> deleting insn with uid = 8.


It looks like it replaced the store with a reg move, and it should
have inserted the store back to memory with the clobber back later in
the CFG.

IE it pushed your store down to some later point in the CFG.

Re: store_motion query

[DJ Delorie]

> It looks like it replaced the store with a reg move, and it should
> have inserted the store back to memory with the clobber back later in
> the CFG.
> 
> IE it pushed your store down to some later point in the CFG.


It's not the store that needs the clobber, it's the shift.  The R8C
can only use shift counts in the UPPER half of $r1, which totally
confuses the register allocator, so I just clobber $r1 in the insn and
do the move myself.

Re: store_motion query

[DJ Delorie]

> But it looks like update_ld_motion_stores() and insert_store() also
> call gen_move_insn without adding clobbers if necessary. I suppose the
> insns produced there should also go through insn_invalid_p somewhere.
> I sent a hack for that to DJ, but I guess the proper place to fix this
> is inside gcse.c itself.

What about putting that logic inside gen_move_insn ?

Re: store_motion query

[Steven Bosscher]

On Thu, May 22, 2008 at 12:06 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Wed, May 21, 2008 at 11:41 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
>> Maybe that should be emit_move_insn()?
>
> OK, so that is not it.
>
> The problem is that can_assign_to_reg_p() returns true when x is
> (ashift:HI (reg/v:HI 27 [ n ]) (subreg:QI (reg/v:HI 27 [ n ]) 0)).
> num_clobbers == 1 but added_clobbers_hard_reg_p returns false.
>
> It seems to me that store motion is OK here, but I don't know who is
> usually responsible for adding the clobbers....

In the rest of gcse.c this works because of process_insert_insn(),
which uses insn_invalid_p() to add clobbers if necessary.

But it looks like update_ld_motion_stores() and insert_store() also
call gen_move_insn without adding clobbers if necessary. I suppose the
insns produced there should also go through insn_invalid_p somewhere.
I sent a hack for that to DJ, but I guess the proper place to fix this
is inside gcse.c itself.

Gr.
Steven

Re: store_motion query

[Steven Bosscher]

On Wed, May 21, 2008 at 11:41 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> Maybe that should be emit_move_insn()?

OK, so that is not it.

The problem is that can_assign_to_reg_p() returns true when x is
(ashift:HI (reg/v:HI 27 [ n ]) (subreg:QI (reg/v:HI 27 [ n ]) 0)).
num_clobbers == 1 but added_clobbers_hard_reg_p returns false.

It seems to me that store motion is OK here, but I don't know who is
usually responsible for adding the clobbers...

Gr.
Steven

Re: store_motion query

[Steven Bosscher]

xf. http://gcc.gnu.org/ml/gcc/2008-05/msg00274.html

> Why is store_motion doing this?
>
> STORE_MOTION  delete insn in BB 2:
>       (insn 8 7 27 2 dj.c:10 (parallel [
>                         (set (mem/s/j:HI (reg/v/f:HI 26 [ s ]) [0 <variable>.buf+0 S2 A8])
>                             (ashift:HI (reg/v:HI 27 [ n ])
>                                 (subreg:QI (reg/v:HI 27 [ n ]) 0)))
>                         (clobber (scratch:HI))
>                     ]) 223 {ashlhi3_i} (expr_list:REG_DEAD (reg/v:HI 27 [ n ])
>                     (nil)))
> STORE MOTION  replaced with insn:
>       (insn 27 8 9 2 dj.c:10 (set (reg:HI 29 [ <variable>.buf ])
> >                     (ashift:HI (reg/v:HI 27 [ n ])
>                         (subreg:QI (reg/v:HI 27 [ n ]) 0))) -1 (expr_list:REG_DEAD (reg/v:HI 27 [ n ])
>                     (nil)))

This insn is generated by gen_move_insn in gcse.c:replace_store_insn().

Maybe that should be emit_move_insn()?

Gr.
Steven