Re: [patch] fix verify_rtl_sharing handling of SEQUENCEs
On Tue, Apr 16, 2013 at 6:39 PM, Steven Bosscher wrote:
>> My new delay branch scheduler uses TODO_verify_rtl_sharing but it
>> turns out that verify_rtl_sharing doesn't handle SEQUENCEs correctly:
>> Clearing the used-flags is done correctly, but verifying insns in the
>> SEQUENCE fails. The problem is that every insn in the SEQUENCE is
>> marked used via PATTERN(SEQUENCE) and also via PREV_INSN/NEXT_INSN of
>> the insns in the SEQUENCE.
>>
>> Fixed with the attached patch. Bootstrapepd&tested on
>> sparc64-unknown-linux-gnu, and cross-built&tested on mips-elf, both
>> with TODO_verify_rtl_sharing added to the passes in reorg.c.
>> Will commit as obvious.
>
> Andreas Krebbel's patch
> (http://gcc.gnu.org/ml/gcc-patches/2013-04/msg00714.html) makes the
> problem go away for me, so I'm not going to commit this patch after
> all.
Hmm, no it hasn't.
What happens is this:
reset_all_used_flags resets the "used" flags via mark_used_flags,
which doesn't mark or unmark insns:
case DEBUG_INSN:
case INSN:
case JUMP_INSN:
case CALL_INSN:
case NOTE:
case LABEL_REF:
case BARRIER:
/* The chain of insns is not being copied. */
return;
But verify_rtx_sharing sets the "used" flag on insns if they are
reached via a SEQUENCE. So the first verify_rtl_sharing call with
SEQUENCEs in the insn chain passes, but a second call will fail
because the "used" flags on insns in the SEQUENCE isn't cleared.
So, updated patch attached, will commit after testing on sparc64-linux.
Ciao!
Steven
* emit-rtl.c (reset_insn_used_flags): New function.
(reset_all_used_flags): Use it.
(verify_insn_sharing): New function.
(verify_rtl_sharing): Fix verification for SEQUENCEs.
Index: emit-rtl.c
===
--- emit-rtl.c (revision 198036)
+++ emit-rtl.c (working copy)
@@ -2596,6 +2596,18 @@ verify_rtx_sharing (rtx orig, rtx insn)
return;
}
+/* Reset used-flags for INSN. */
+
+static void
+reset_insn_used_flags (rtx insn)
+{
+ gcc_assert (INSN_P (insn));
+ reset_used_flags (PATTERN (insn));
+ reset_used_flags (REG_NOTES (insn));
+ if (CALL_P (insn))
+reset_used_flags (CALL_INSN_FUNCTION_USAGE (insn));
+}
+
/* Go through all the RTL insn bodies and clear all the USED bits. */
static void
@@ -2606,28 +2618,30 @@ reset_all_used_flags (void)
for (p = get_insns (); p; p = NEXT_INSN (p))
if (INSN_P (p))
{
- reset_used_flags (PATTERN (p));
- reset_used_flags (REG_NOTES (p));
- if (CALL_P (p))
- reset_used_flags (CALL_INSN_FUNCTION_USAGE (p));
- if (GET_CODE (PATTERN (p)) == SEQUENCE)
- {
- int i;
- rtx q, sequence = PATTERN (p);
-
- for (i = 0; i < XVECLEN (sequence, 0); i++)
- {
- q = XVECEXP (sequence, 0, i);
- gcc_assert (INSN_P (q));
- reset_used_flags (PATTERN (q));
- reset_used_flags (REG_NOTES (q));
- if (CALL_P (q))
- reset_used_flags (CALL_INSN_FUNCTION_USAGE (q));
- }
- }
+ rtx pat = PATTERN (p);
+ if (GET_CODE (pat) != SEQUENCE)
+ reset_insn_used_flags (p);
+ else
+ {
+ gcc_assert (REG_NOTES (p) == NULL);
+ for (int i = 0; i < XVECLEN (pat, 0); i++)
+ reset_insn_used_flags (XVECEXP (pat, 0, i));
+ }
}
}
+/* Verify sharing in INSN. */
+
+static void
+verify_insn_sharing (rtx insn)
+{
+ gcc_assert (INSN_P (insn));
+ reset_used_flags (PATTERN (insn));
+ reset_used_flags (REG_NOTES (insn));
+ if (CALL_P (insn))
+reset_used_flags (CALL_INSN_FUNCTION_USAGE (insn));
+}
+
/* Go through all the RTL insn bodies and check that there is no unexpected
sharing in between the subexpressions. */
@@ -2643,10 +2657,12 @@ verify_rtl_sharing (void)
for (p = get_insns (); p; p = NEXT_INSN (p))
if (INSN_P (p))
{
- verify_rtx_sharing (PATTERN (p), p);
- verify_rtx_sharing (REG_NOTES (p), p);
- if (CALL_P (p))
- verify_rtx_sharing (CALL_INSN_FUNCTION_USAGE (p), p);
+ rtx pat = PATTERN (p);
+ if (GET_CODE (pat) != SEQUENCE)
+ verify_insn_sharing (p);
+ else
+ for (i = 0; i < XVECLEN (pat, 0); i++)
+ verify_insn_sharing (XVECEXP (pat, 0, i));
}
reset_all_used_flags ();
Re: [patch] fix verify_rtl_sharing handling of SEQUENCEs
On Sun, Apr 14, 2013 at 3:34 PM, Steven Bosscher wrote: > Hello, > > My new delay branch scheduler uses TODO_verify_rtl_sharing but it > turns out that verify_rtl_sharing doesn't handle SEQUENCEs correctly: > Clearing the used-flags is done correctly, but verifying insns in the > SEQUENCE fails. The problem is that every insn in the SEQUENCE is > marked used via PATTERN(SEQUENCE) and also via PREV_INSN/NEXT_INSN of > the insns in the SEQUENCE. > > Fixed with the attached patch. Bootstrapepd&tested on > sparc64-unknown-linux-gnu, and cross-built&tested on mips-elf, both > with TODO_verify_rtl_sharing added to the passes in reorg.c. > Will commit as obvious. Andreas Krebbel's patch (http://gcc.gnu.org/ml/gcc-patches/2013-04/msg00714.html) makes the problem go away for me, so I'm not going to commit this patch after all. Ciao! Steven
[patch] fix verify_rtl_sharing handling of SEQUENCEs
Hello,
My new delay branch scheduler uses TODO_verify_rtl_sharing but it
turns out that verify_rtl_sharing doesn't handle SEQUENCEs correctly:
Clearing the used-flags is done correctly, but verifying insns in the
SEQUENCE fails. The problem is that every insn in the SEQUENCE is
marked used via PATTERN(SEQUENCE) and also via PREV_INSN/NEXT_INSN of
the insns in the SEQUENCE.
Fixed with the attached patch. Bootstrapepd&tested on
sparc64-unknown-linux-gnu, and cross-built&tested on mips-elf, both
with TODO_verify_rtl_sharing added to the passes in reorg.c.
Will commit as obvious.
Ciao!
Steven
* emit-rtl.c (reset_insn_used_flags): New function.
(verify_insn_sharing): New function.
(verify_rtl_sharing): Fix marking and verification of insns.
Index: emit-rtl.c
===
--- emit-rtl.c (revision 197944)
+++ emit-rtl.c (working copy)
@@ -2596,47 +2596,64 @@ verify_rtx_sharing (rtx orig, rtx insn)
return;
}
+/* Reset used-flags for INSN. */
+
+static void
+reset_insn_used_flags (rtx insn)
+{
+ gcc_assert (INSN_P (insn));
+ reset_used_flags (PATTERN (insn));
+ reset_used_flags (REG_NOTES (insn));
+ if (CALL_P (insn))
+reset_used_flags (CALL_INSN_FUNCTION_USAGE (insn));
+}
+
+/* Verify sharing in INSN. */
+
+static void
+verify_insn_sharing (rtx insn)
+{
+ gcc_assert (INSN_P (insn));
+ reset_used_flags (PATTERN (insn));
+ reset_used_flags (REG_NOTES (insn));
+ if (CALL_P (insn))
+reset_used_flags (CALL_INSN_FUNCTION_USAGE (insn));
+}
+
/* Go through all the RTL insn bodies and check that there is no unexpected
sharing in between the subexpressions. */
DEBUG_FUNCTION void
verify_rtl_sharing (void)
{
- rtx p;
+ rtx p, pat;
+ int i;
timevar_push (TV_VERIFY_RTL_SHARING);
for (p = get_insns (); p; p = NEXT_INSN (p))
if (INSN_P (p))
{
- reset_used_flags (PATTERN (p));
- reset_used_flags (REG_NOTES (p));
- if (CALL_P (p))
- reset_used_flags (CALL_INSN_FUNCTION_USAGE (p));
- if (GET_CODE (PATTERN (p)) == SEQUENCE)
+rtx pat = PATTERN (p);
+if (GET_CODE (pat) != SEQUENCE)
+ reset_insn_used_flags (p);
+ else
{
- int i;
- rtx q, sequence = PATTERN (p);
-
- for (i = 0; i < XVECLEN (sequence, 0); i++)
- {
- q = XVECEXP (sequence, 0, i);
- gcc_assert (INSN_P (q));
- reset_used_flags (PATTERN (q));
- reset_used_flags (REG_NOTES (q));
- if (CALL_P (q))
- reset_used_flags (CALL_INSN_FUNCTION_USAGE (q));
- }
+ gcc_assert (REG_NOTES (p) == NULL);
+ for (i = 0; i < XVECLEN (pat, 0); i++)
+ reset_insn_used_flags (XVECEXP (pat, 0, i));
}
}
for (p = get_insns (); p; p = NEXT_INSN (p))
if (INSN_P (p))
{
- verify_rtx_sharing (PATTERN (p), p);
- verify_rtx_sharing (REG_NOTES (p), p);
- if (CALL_P (p))
- verify_rtx_sharing (CALL_INSN_FUNCTION_USAGE (p), p);
+rtx pat = PATTERN (p);
+if (GET_CODE (pat) != SEQUENCE)
+ verify_insn_sharing (p);
+ else
+ for (i = 0; i < XVECLEN (pat, 0); i++)
+ verify_insn_sharing (XVECEXP (pat, 0, i));
}
timevar_pop (TV_VERIFY_RTL_SHARING);
