Re: Your last commit to new-regalloc-branch

[Daniel Berlin <dan at cgsoftware dot com>]

Denis Chertykov <denisc@overta.ru> writes:

> Daniel !
> 
> 2001-08-03  Daniel Berlin  <dan@cgsoftware.com>
> 
> 	* ra.c (coalesce): Comment out conservative test, in preparation
> 	for optimistic coalescing. Also fixes x86 (though this is a side effect).
> 
> You kill all my changes !!!
Accident, I was stupid and sick, and shouldn't have been coding.
Sorry about that.

> 
> Wed Aug  1 23:40:05 2001  Denis Chertykov  <denisc@overta.ru>
> 
> 	* ra.c (undef_to_bitmap): Handle more variants of *undefined.
> 
> Wed Aug  1 23:35:02 2001  Denis Chertykov  <denisc@overta.ru>
> 
> 	* ra.c (combine): combine add_hardregs's of U and V.
> 	(ok): Remove check for combining a web with a precolored web.
> 	Check a precolored web for usable_regs.
> 
> 	(rewrite_program): Since we always call `regclass' if
> 	`changed' we can emit a new pseudos which will have a new reg
> 	class preferences.
> 
> ----------------------------------------------------------------------
> 
>> This one is your fault (or rather, caused by your patch)
>> Reverting your latest patch makes it go away.
> 
> I was wrong here:
> 
> 	(rewrite_program): Since we always call `regclass' if
> 	`changed' we can emit a new pseudos which will have a new reg
> 	class preferences.
> 
> Call to `regclass' in `reg_alloc' produce wrong register
> preferences if new pseudo registers was added.
It shouldn't.

> 
>> I suspect we are causing too many conflicts on x86, and because we can
>> coalesce more, we keep coalescing ourselves into the same position
>> over and over and over.
> 
> IMHO: You wrong here.
> Bug occurs because rewrite_program emit new pseudo registers.
> 
> The following code don't produce proper regclass preferences.
> Fragment from reg_alloc:
> 
>  	  allocate_reg_info (max_reg_num (), FALSE, TRUE);
>  	  reg_scan_update (get_insns(), BLOCK_END (n_basic_blocks - 1),
>  			   max_regno);
>   	  regclass (get_insns (), max_reg_num (), rtl_dump_file);
> 
> 
> Try the following patch against ra.c version 1.1.2.28
> 
> Sat Aug  4 16:50:57 2001  Denis Chertykov  <denisc@overta.ru>
> 
> 	* ra.c (reg_alloc): Rebuild register preferences.
> 	(rewrite_program): correct `max_regno'.
> 
> 
> 
> Index: ra.c
> ===================================================================
> RCS file: /cvs/gcc/egcs/gcc/Attic/ra.c,v
> retrieving revision 1.1.2.28
> diff -c -3 -p -r1.1.2.28 ra.c
> *** ra.c	2001/08/01 19:53:24	1.1.2.28
> --- ra.c	2001/08/04 12:48:29
> *************** ok (target, source)
> *** 2832,2838 ****
>     for (i = target->add_hardregs; i >= 0; --i)
>       if (!TEST_HARD_REG_BIT (usable_regs[target->regclass], source->color + i))
>         return 0;
> ! 
>     for (wl = target->conflict_list; wl; wl = wl->next)
>       {
>         struct web *pweb = wl->t;
> --- 2832,2838 ----
>     for (i = target->add_hardregs; i >= 0; --i)
>       if (!TEST_HARD_REG_BIT (usable_regs[target->regclass], source->color + i))
>         return 0;
> !  
>     for (wl = target->conflict_list; wl; wl = wl->next)
>       {
>         struct web *pweb = wl->t;
> *************** rewrite_program (void)
> *** 3697,3703 ****
>   		{
>   		  rtx reg = gen_reg_rtx (GET_MODE (source));
>   		  if (validate_change (insn, DF_REF_LOC (web->defs[j]),
> ! 					reg, 0))
>   		    emit_insn (gen_move_insn (dest, reg));
>   		  else
>   		    emit_insn (gen_move_insn (dest, source));
> --- 3697,3703 ----
>   		{
>   		  rtx reg = gen_reg_rtx (GET_MODE (source));
>   		  if (validate_change (insn, DF_REF_LOC (web->defs[j]),
> ! 				       reg, 0))
>   		    emit_insn (gen_move_insn (dest, reg));
>   		  else
>   		    emit_insn (gen_move_insn (dest, source));
> *************** rewrite_program (void)
> *** 3770,3776 ****
>   		  rtx reg = gen_reg_rtx (GET_MODE (target));
>   
>   		  if (validate_change (insn, DF_REF_LOC (web->uses[j]),
> ! 					reg, 0))
>   		    emit_insn (gen_move_insn (reg ,source));
>   		  else
>   		    emit_insn (gen_move_insn (target, source));
> --- 3770,3776 ----
>   		  rtx reg = gen_reg_rtx (GET_MODE (target));
>   
>   		  if (validate_change (insn, DF_REF_LOC (web->uses[j]),
> ! 				       reg, 0))
>   		    emit_insn (gen_move_insn (reg ,source));
>   		  else
>   		    emit_insn (gen_move_insn (target, source));
> *************** rewrite_program (void)
> *** 3790,3795 ****
> --- 3790,3797 ----
>      }
>   
>     BITMAP_XFREE (b);
>+ 
>+   max_regno = max_reg_num ();
>     
>     /*if (! validate_change (insn, df->defs[i]->loc, web->stack_slot, 0)) */
>  }
> *************** reg_alloc (void)
> *** 4199,4207 ****
>  	}
>         else
>   	{
> ! 	  allocate_reg_info (max_reg_num (), FALSE, TRUE);
> ! 	  reg_scan_update (get_insns(), BLOCK_END (n_basic_blocks - 1),
> ! 			   max_regno);
>   	  regclass (get_insns (), max_reg_num (), rtl_dump_file);
>  	}
>         dump_ra (df);
> --- 4201,4212 ----
>  	}
>         else
>   	{
> ! 	  clear_log_links (get_insns ());
> ! 	  life_analysis (get_insns (), rtl_dump_file, PROP_REG_INFO);
> ! 	  compute_bb_for_insn (get_max_uid ());
> ! 	  reg_scan (get_insns (), max_reg_num (), 1);
> ! 	  recompute_reg_usage (get_insns (), TRUE);
> ! 	  regclass_init ();
>   	  regclass (get_insns (), max_reg_num (), rtl_dump_file);
>  	}
>         dump_ra (df);


If this works, it means we need to fix regclass and regscan_update to
just frickin work.
We can't go redoing all of life analysis, all of register scanning,
all of register usage, etc, on each pass.
It's murder.

reg_scan_update was specifically there for incremental updating when
you emit new pseudos.
If it doesn't work, we should fix it.

max_regno was also correct given the circumstances. reg_scan_update
wanted the previous max register (IE the max before we emitted new
pseudos), so max_regno, having not been updated, was that number.

--Dan

-- 
"I saw a bank that said "24 Hour Banking", but I don't have that
much time.
"-Steven Wright