Patchwork Fix var-tracking ICE (PR debug/46387)

login
register
mail settings
Submitter Alexander Monakov
Date Nov. 11, 2010, 3:32 p.m.
Message ID <alpine.LNX.2.00.1011111822410.6883@monoid.intra.ispras.ru>
Download mbox | patch
Permalink /patch/70825/
State New
Headers show

Comments

Alexander Monakov - Nov. 11, 2010, 3:32 p.m.
Jakub, Richard,

This is what we need from the selective scheduler side (the part of the
sel-sched.c patch actually needs to be more complicated to work properly, but
the alias.c change is what we have in mind to fix PR45652).

Thanks for your help.
Jakub Jelinek - Nov. 11, 2010, 3:41 p.m.
On Thu, Nov 11, 2010 at 06:32:28PM +0300, Alexander Monakov wrote:
> This is what we need from the selective scheduler side (the part of the
> sel-sched.c patch actually needs to be more complicated to work properly, but
> the alias.c change is what we have in mind to fix PR45652).

That's unfortunately not usable for var-tracking, where we know
VEC_index (rtx, reg_base_value, dest_regno) is non-NULL and different from
src_regno, yet we want to copy from src_regno instead of clearing it.

> +/* Set REG_BASE_VALUE of DEST to that of SRC if unset,
> +   or set it to NULL if DEST and SRC have conflicting REG_BASE_VALUEs.  */
> +void
> +inherit_reg_base_value (rtx dest, rtx src)
> +{
> +  unsigned dest_regno = REGNO (dest), src_regno = REGNO (src);
> +
> +  if (! VEC_index (rtx, reg_base_value, dest_regno))
> +    {
> +      if (!reg_seen[dest_regno]
> +         && (dest_regno >= FIRST_PSEUDO_REGISTER || !fixed_regs[dest_regno]))
> +       VEC_replace (rtx, reg_base_value, dest_regno,
> +                    VEC_index (rtx, reg_base_value, src_regno));
> +    }
> +  else if (VEC_index (rtx, reg_base_value, src_regno)
> +          != VEC_index (rtx, reg_base_value, dest_regno))
> +    VEC_replace (rtx, reg_base_value, dest_regno, NULL_RTX);
> +
> +  reg_seen[dest_regno] = 1;
> +}
> +

	Jakub
Richard Guenther - Nov. 12, 2010, 9:57 a.m.
On Thu, Nov 11, 2010 at 4:41 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Nov 11, 2010 at 06:32:28PM +0300, Alexander Monakov wrote:
>> This is what we need from the selective scheduler side (the part of the
>> sel-sched.c patch actually needs to be more complicated to work properly, but
>> the alias.c change is what we have in mind to fix PR45652).
>
> That's unfortunately not usable for var-tracking, where we know
> VEC_index (rtx, reg_base_value, dest_regno) is non-NULL and different from
> src_regno, yet we want to copy from src_regno instead of clearing it.

Hm, can't we teach alias analysis about cfa_base_reg somehow?  What would
we loose (in var-tracking) if we re-set the regs base value to NULL instead
of to that of the frame pointer?

Richard.

>> +/* Set REG_BASE_VALUE of DEST to that of SRC if unset,
>> +   or set it to NULL if DEST and SRC have conflicting REG_BASE_VALUEs.  */
>> +void
>> +inherit_reg_base_value (rtx dest, rtx src)
>> +{
>> +  unsigned dest_regno = REGNO (dest), src_regno = REGNO (src);
>> +
>> +  if (! VEC_index (rtx, reg_base_value, dest_regno))
>> +    {
>> +      if (!reg_seen[dest_regno]
>> +         && (dest_regno >= FIRST_PSEUDO_REGISTER || !fixed_regs[dest_regno]))
>> +       VEC_replace (rtx, reg_base_value, dest_regno,
>> +                    VEC_index (rtx, reg_base_value, src_regno));
>> +    }
>> +  else if (VEC_index (rtx, reg_base_value, src_regno)
>> +          != VEC_index (rtx, reg_base_value, dest_regno))
>> +    VEC_replace (rtx, reg_base_value, dest_regno, NULL_RTX);
>> +
>> +  reg_seen[dest_regno] = 1;
>> +}
>> +
>
>        Jakub
>
Jakub Jelinek - Nov. 12, 2010, 10:55 a.m.
On Fri, Nov 12, 2010 at 10:57:06AM +0100, Richard Guenther wrote:
> On Thu, Nov 11, 2010 at 4:41 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Thu, Nov 11, 2010 at 06:32:28PM +0300, Alexander Monakov wrote:
> >> This is what we need from the selective scheduler side (the part of the
> >> sel-sched.c patch actually needs to be more complicated to work properly, but
> >> the alias.c change is what we have in mind to fix PR45652).
> >
> > That's unfortunately not usable for var-tracking, where we know
> > VEC_index (rtx, reg_base_value, dest_regno) is non-NULL and different from
> > src_regno, yet we want to copy from src_regno instead of clearing it.
> 
> Hm, can't we teach alias analysis about cfa_base_reg somehow?  What would

Ugh, by repeating all the checks var-tracking does for it?  That would be
very ugly and very expensive.  Remember the tests include e.g.
eliminate_regs.  If sel-sched invokes this function itself, it would be very
slow.

> we loose (in var-tracking) if we re-set the regs base value to NULL instead
> of to that of the frame pointer?

Suddenly cfa_base_rtx based mems would be thought to alias everything, which
would make debug info quality significantly worse.

I must say I don't understand why the patch sets it to NULL_RTX instead of
copying there VEC_index (rtx, reg_base_value, src_regno), by making both
have the same value they are handled like they might alias each other,
but still not considered to alias unrelated bases.

Would sel-sched work with my alias.c change (i.e. just the VEC_replace)?

If not, perhaps we should add some additional argument which would select
the behavior we want (but that is not very far from just having two
different functions).

	Jakub
Alexander Monakov - Nov. 12, 2010, 11:15 a.m.
On Fri, 12 Nov 2010, Jakub Jelinek wrote:

> I must say I don't understand why the patch sets it to NULL_RTX instead of
> copying there VEC_index (rtx, reg_base_value, src_regno), by making both
> have the same value they are handled like they might alias each other,
> but still not considered to alias unrelated bases.

If dest_regno already has a reg_base_value, it may have a SET somewhere else
in the function.  In selective scheduler, we are creating a new SET, and if we
just copy reg_base_value from src_regno, we would be changing alias
information for the existing SET of the same register.  It would be wrong: the
existing SET may alias some bases that the new would not.  Therefore, we have
to drop it to NULL.

> Would sel-sched work with my alias.c change (i.e. just the VEC_replace)?

No, we reuse registers for renaming, and per the above, we cannot simply
replace.

Thanks.

Alexander
Richard Guenther - Nov. 12, 2010, 11:54 a.m.
On Fri, Nov 12, 2010 at 12:15 PM, Alexander Monakov <amonakov@ispras.ru> wrote:
>
>
> On Fri, 12 Nov 2010, Jakub Jelinek wrote:
>
>> I must say I don't understand why the patch sets it to NULL_RTX instead of
>> copying there VEC_index (rtx, reg_base_value, src_regno), by making both
>> have the same value they are handled like they might alias each other,
>> but still not considered to alias unrelated bases.
>
> If dest_regno already has a reg_base_value, it may have a SET somewhere else
> in the function.  In selective scheduler, we are creating a new SET, and if we
> just copy reg_base_value from src_regno, we would be changing alias
> information for the existing SET of the same register.  It would be wrong: the
> existing SET may alias some bases that the new would not.  Therefore, we have
> to drop it to NULL.

Yep, you'd need to unify the base values, thus, loop over all reg_base_values
and change each occurance of either to the other.  That would work
I think.  Don't you need something similar for the cfa_base_reg?

Richard.
Jakub Jelinek - Nov. 12, 2010, 12:05 p.m.
On Fri, Nov 12, 2010 at 12:54:31PM +0100, Richard Guenther wrote:
> Yep, you'd need to unify the base values, thus, loop over all reg_base_values
> and change each occurance of either to the other.  That would work

If the loop is needed, then it would be needed for both the NULL and copy
alternative, if the loop is not needed, it is needed for neither variant.
REG_BASE_VALUE (dest) = NULL_RTX is changing just that one reg to point to
anything, but any other reg which has its old REG_BASE_VALUE too will
still contain its old REG_BASE_VALUE.

> I think.  Don't you need something similar for the cfa_base_reg?

I don't, because I know precisely what REG_BASE_VALUE (cfa_base_rtx) is
(it is an ADDRESS not used anywhere else).

	Jakub

Patch

diff --git a/gcc/alias.c b/gcc/alias.c
index 2e0ac06..0e65d66 100644
--- a/gcc/alias.c
+++ b/gcc/alias.c
@@ -1291,6 +1291,27 @@  record_set (rtx dest, const_rtx set, void *data ATTRIBUTE_UNUSED)
   reg_seen[regno] = 1;
 }

+/* Set REG_BASE_VALUE of DEST to that of SRC if unset,
+   or set it to NULL if DEST and SRC have conflicting REG_BASE_VALUEs.  */
+void
+inherit_reg_base_value (rtx dest, rtx src)
+{
+  unsigned dest_regno = REGNO (dest), src_regno = REGNO (src);
+
+  if (! VEC_index (rtx, reg_base_value, dest_regno))
+    {
+      if (!reg_seen[dest_regno]
+         && (dest_regno >= FIRST_PSEUDO_REGISTER || !fixed_regs[dest_regno]))
+       VEC_replace (rtx, reg_base_value, dest_regno,
+                    VEC_index (rtx, reg_base_value, src_regno));
+    }
+  else if (VEC_index (rtx, reg_base_value, src_regno)
+          != VEC_index (rtx, reg_base_value, dest_regno))
+    VEC_replace (rtx, reg_base_value, dest_regno, NULL_RTX);
+
+  reg_seen[dest_regno] = 1;
+}
+
 /* If a value is known for REGNO, return it.  */

 rtx
@@ -2901,14 +2922,14 @@  init_alias_analysis (void)
   /* Clean up.  */
   free (new_reg_base_value);
   new_reg_base_value = 0;
-  free (reg_seen);
-  reg_seen = 0;
   timevar_pop (TV_ALIAS_ANALYSIS);
 }

 void
 end_alias_analysis (void)
 {
+  free (reg_seen);
+  reg_seen = 0;
   old_reg_base_value = reg_base_value;
   ggc_free (reg_known_value);
   reg_known_value = 0;
diff --git a/gcc/rtl.h b/gcc/rtl.h
index 3e1df2c..8cc6c5c 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -2508,6 +2508,7 @@  extern rtx find_base_term (rtx);
 extern rtx gen_hard_reg_clobber (enum machine_mode, unsigned int);
 extern rtx get_reg_known_value (unsigned int);
 extern bool get_reg_known_equiv_p (unsigned int);
+extern void inherit_reg_base_value (rtx, rtx);

 #ifdef STACK_REGS
 extern int stack_regs_mentioned (const_rtx insn);
diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
index 70e831d..ab7c708 100644
--- a/gcc/sel-sched.c
+++ b/gcc/sel-sched.c
@@ -5856,6 +5856,7 @@  maybe_emit_renaming_copy (rtx insn,
                                                    insn);
       EXPR_SPEC_DONE_DS (INSN_EXPR (reg_move_insn)) = 0;
       replace_dest_with_reg_in_expr (params->c_expr, params->dest);
+      inherit_reg_base_value (params->dest, cur_reg);

       insn_emitted = true;
       params->was_renamed = true;