[jit] Fix state issue in gcse.c

On Sat, Apr 19, 2014 at 3:24 PM, Steven Bosscher wrote:
> On Tue, Mar 11, 2014 at 8:00 PM, David Malcolm wrote:
>> Investigation revealed the issue to be a CFG from the previous compile
>> being kept alive by this GC root in gcse.c:
>>           static GTY(()) rtx test_insn;
>>
>> This wouldn't it itself be an issue, but one (or more) of the edges had:
>
> But this is an issue! This is not supposed to be possible.
>
> test_insn is not in the insns chain and also not in a basic block, so
> it should never keep a CFG alive.
>
> Your patch papers over a bigger issue: How does test_insn end up
> preventing the CFG from being garbage-collected.


The only way this could happen, that I can think of, is a reference
from SET_SRC into the insn stream (like a label_ref).

Can you please try and see if the patch below fixes your problem?

Ciao!
Steven



        * gcse.c (can_assign_to_reg_without_clobbers_p): Do not let pointers
        from test_insn into GGC space escape via SET_SRC.

On Sat, 2014-04-19 at 15:55 +0200, Steven Bosscher wrote:
> On Sat, Apr 19, 2014 at 3:24 PM, Steven Bosscher wrote:
> > On Tue, Mar 11, 2014 at 8:00 PM, David Malcolm wrote:
> >> Investigation revealed the issue to be a CFG from the previous compile
> >> being kept alive by this GC root in gcse.c:
> >>           static GTY(()) rtx test_insn;
> >>
> >> This wouldn't it itself be an issue, but one (or more) of the edges had:
> >
> > But this is an issue! This is not supposed to be possible.
> >
> > test_insn is not in the insns chain and also not in a basic block, so
> > it should never keep a CFG alive.
> >
> > Your patch papers over a bigger issue: How does test_insn end up
> > preventing the CFG from being garbage-collected.
> 
> 
> The only way this could happen, that I can think of, is a reference
> from SET_SRC into the insn stream (like a label_ref).
> 
> Can you please try and see if the patch below fixes your problem?

Thanks; your patch does indeed fix the issue: with it, I no longer need
the band-aid from my patch to be able to run the jit testsuite with full
optimization.

> 
> Ciao!
> Steven
> 
> 
> 
>         * gcse.c (can_assign_to_reg_without_clobbers_p): Do not let pointers
>         from test_insn into GGC space escape via SET_SRC.
> 
> Index: gcse.c
> ===================================================================
> --- gcse.c      (revision 209530)
> +++ gcse.c      (working copy)
> @@ -849,6 +849,7 @@ can_assign_to_reg_without_clobbers_p (rtx x)
>  {
>    int num_clobbers = 0;
>    int icode;
> +  bool can_assign = false;
> 
>    /* If this is a valid operand, we are OK.  If it's VOIDmode, we aren't.  */
>    if (general_operand (x, GET_MODE (x)))
> @@ -866,6 +867,7 @@ can_assign_to_reg_without_clobbers_p (rtx x)
>                                                    FIRST_PSEUDO_REGISTER * 2),
>                                       const0_rtx));
>        NEXT_INSN (test_insn) = PREV_INSN (test_insn) = 0;
> +      INSN_LOCATION (test_insn) = UNKNOWN_LOCATION;
>      }
> 
>    /* Now make an insn like the one we would make when GCSE'ing and see if
> @@ -874,16 +876,19 @@ can_assign_to_reg_without_clobbers_p (rtx x)
>    SET_SRC (PATTERN (test_insn)) = x;
> 
>    icode = recog (PATTERN (test_insn), test_insn, &num_clobbers);
> -  if (icode < 0)
> -    return false;
> -
> -  if (num_clobbers > 0 && added_clobbers_hard_reg_p (icode))
> -    return false;
> -
> -  if (targetm.cannot_copy_insn_p && targetm.cannot_copy_insn_p (test_insn))
> -    return false;
> -
> -  return true;
> +
> +  /* If the test insn is valid and doesn't need clobbers, and the target also
> +     has no objections, we're good.  */
> +  if (icode != 0
> +      && (num_clobbers == 0 || !added_clobbers_hard_reg_p (icode))
> +      && ! (targetm.cannot_copy_insn_p
> +           && targetm.cannot_copy_insn_p (test_insn)))
> +    can_assign = true;
> +
> +  /* Make sure test_insn doesn't have any pointers into GC space.  */
> +  SET_SRC (PATTERN (test_insn)) = NULL_RTX;
> +
> +  return can_assign;
>  }
> 
>  /* Return nonzero if the operands of expression X are unchanged from the

On Fri, Apr 25, 2014 at 9:48 PM, David Malcolm wrote:
> Thanks; your patch does indeed fix the issue: with it, I no longer need
> the band-aid from my patch to be able to run the jit testsuite with full
> optimization.

Good. In the mean time I tested the patch, and found a typo:

+  if (icode != 0

should be:

+  if (icode >= 0

I'll post the patch for trunk this weekend.

Ciao!
Steven