Re: [PATCH] Fix DSE (PR middle-end/39794)

[Jakub Jelinek <jakub at redhat dot com>]

On Thu, Apr 23, 2009 at 05:11:23PM +0200, Eric Botcazou wrote:
> > Seems sched-deps.c uses cselib_subst_to_values for these purposes, so if
> > the REGs mentioned in the MEM actually change in between, the addresses are
> > different and thus canon_true_dependence does the right thing.
> 
> sched-deps.c doesn't store cselib values locally whereas dse.c does though.
> 
> What about something like:
> 
>   if (spill_alias_set || group_id < 0)
>     mem_addr = base->val_rtx;

Yeah, I guess this can work for group_id < 0 (will test soon).
For spill_alias_set that doesn't work though, as base is NULL in that case.

But it seems for spill_alias_set mems canon_true_dependence (the only places
where ->mem_addr or mem_addr is ever used) is never used:
1) in record_store the only canon_true_dependence call is in:
   if (s_info->alias_set != spill_alias_set)
     del = false;
   else if (s_info->alias_set)
     {
       ...
     }
   else if (...)
     {
       ...
     }
   else if (...)
     {
       ... canon_true_dependence (...) ...
     }
   so if either s_info->alias_set or spill_alias_set is non-zero, it won't
   be called.
2) in check_mem_read_rtx one there is:
   if (spill_alias_set)
     {
       ...
     }
   else if (...)
     {
       ...
       if (store_info->group_id < 0)
	 canon_true_dependence (...)
       else if (group_id == store_info->group_id)
	 canon_true_dependence (...)
       ...
     }
   else
     {
       ...
       if (!store_info->alias_set)
	 canon_true_dependence (...)
       ...
     }
   and as for non-zero ->alias_set/spill_alias_set ->group_id/group_id is
   >= 0, I think canon_true_dependence is never called if either
   spill_alias_set or ->alias_set is non-zero.
Should I set mem_addr to NULL in that case, so that it quickly ICEs if that
isn't true?

>  if (offset)
>    mem_addr = plus_constant (mem_addr, offset);

Shame on me for forgetting about plus_constant.

> and changing
> 
>   /* The result of get_addr on mem.  */
>   rtx mem_addr;
> 
> so that get_addr is precisely _not_ called on mem_addr.

It is not called, just the comment needs updating.

> > The remaining canon_true_dependence in scan_reads_nospill is I think fine,
> > group->canon_base_mem in that case is either a constant address, or frame
> > related and so if registers in read_info->mem change over the life time
> > of the function, group->canon_base_mem does not and so
> > canon_true_dependence should IMHO return the right thing.
> 
> group->canon_base_mem is a MEM, not an address:
> 
>       gi->base_mem = gen_rtx_MEM (QImode, base);
>       gi->canon_base_mem = canon_rtx (gi->base_mem);
> 
> so it's wrong too, but differently.

As the only place where canon_base_mem is used is canon_true_dependence
call, I guess it should be
  gi->canon_base_addr = canon_rtx (base);
then.

	Jakub