This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch, ira] Invalid assert in reload1.c::finish_spills?


On 8/7/19 8:15 AM, Vladimir Makarov wrote:
> On 8/7/19 7:36 AM, SenthilKumar.Selvaraj@microchip.com wrote:
>> Hi,
>>
>>    gcc/testsuite/c-c++-common/pr60101.c fails with an ICE for the
>>    avr target, because of a gcc_assert firing at reload1.c:4233
>>
>>    The assert (in the patch below) looks bogus to me, as it's in
>>    the if block of
>>
>>      if (! ira_conflicts_p || reg_renumber[i] >= 0)
>>
>>    For this testcase and for the avr target, ira_conflicts_p is
>>    false because build_conflict_bit_table bailed out early
>>    (conflict table too big).
>>    If reg_renumber[i] is now negative, the assert fires and causes
>>    the ICE.
>>
>>    Getting rid of the assert (patch below) makes the ICE go away,
>>    not sure if that's the right fix though.
>>
>>    Comments?
> 
> Mike Matz is right.  Removing the assertion will make the bug even worse
> (changing memory beyond pseudo_previous_regs).
> 
> I did some investigation.  This bug occurred from a 10 years old patch
> avoiding building big conflict tables in IRA.  And the assert was in
> reload before IRA.
> 
> I think the solution should be
> 
> Index: reload1.c
> ===================================================================
> --- reload1.c   (revision 273762)
> +++ reload1.c   (working copy)
> @@ -4225,13 +4225,8 @@ finish_spills (int global)
>        spill_reg_order[i] = -1;
> 
>    EXECUTE_IF_SET_IN_REG_SET (&spilled_pseudos, FIRST_PSEUDO_REGISTER,
> i, rsi)
> -    if (! ira_conflicts_p || reg_renumber[i] >= 0)
> +    if (reg_renumber[i] >= 0)
>        {
> -       /* Record the current hard register the pseudo is allocated to
> -          in pseudo_previous_regs so we avoid reallocating it to the
> -          same hard reg in a later pass.  */
> -       gcc_assert (reg_renumber[i] >= 0);
> -
>         SET_HARD_REG_BIT (pseudo_previous_regs[i], reg_renumber[i]);
>         /* Mark it as no longer having a hard register home.  */
>         reg_renumber[i] = -1;
> 
> So if the patch works, you can commit it to the trunk.
I've committed it after letting it bootstrap/regression test on ppc64le,
aarch64 and others.  It also didn't regress on any of the *-elf targets
I'm testing which are far more likely to exercise this code.

jeff
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]