This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
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
>