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

[Jeff Law]

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
>