Ping: IRA-based register pressure calculation for RTL loop invariant motion

[Vladimir Makarov]

Zdenek Dvorak wrote:
> Hi,
>   
Zdenek, thanks for quick response and very useful review.
>   
>> The patch was posted on
>>
>> http://gcc.gnu.org/ml/gcc-patches/2009-09/msg01889.html
>>     
>
> in gain_for_invariant:
>
>   
>> +      for (i = 0; i < ira_reg_class_cover_size; i++)
>> +	{
>> +	  cover_class = ira_reg_class_cover[i];
>> +	  if ((diff = ((int) new_regs[cover_class]
>> +		       + (int) regs_needed[cover_class]
>> +		       + LOOP_DATA (curr_loop)->max_reg_pressure[cover_class]
>> +		       + IRA_LOOP_RESERVED_REGS
>> +		       - ira_available_class_regs[cover_class])) > 0)
>> +	    break;
>> +	}
>>     
>
> diff is not used.
>
>   
I missed this.  Diff was used for experimental code (I  tried a  lot of 
heuristics) which was based one spill cost vs invariant cost.  But I've 
rejected this code at some point.
>> +      if (i < ira_reg_class_cover_size)
>> +	size_cost = comp_cost + 10;
>> +      else
>> +	size_cost = 0;
>>     
>
> Including comp_cost in size_cost makes no sense (this would prevent us from
> moving even very costly invariants out of the loop if we run out of registers).
>
>   
That is exactly what I intended.  As I wrote above, I tried a lot of 
heuristics with different parameters which decided to move loop 
invariant depending on spill cost and loop invariant cost.  But they 
don't  work well at least for x86/x86_64 and power6.  I have some 
speculation for this.  x86/x86_64 is OOO processors these days.  And 
costly invariant will be hidden because usually the invariant has a lot 
of freedom to be executed out-of-order.  For power6, long latency is 
hidden by insn scheduling.  It is hard to me find a processor where it 
will be important.  Another reason for this, it is very hard to evaluate 
accurately spill cost at this stage.  So I decided not to use 
combination of register pressure and invariant cost in my approach.

I any case I've changed the code a bit (to rid of the magic number) and 
add the comment (see the attachment).
> Also, the magic constant 10 requires some explanation (especially since the
> costs elsewhere in the invariant motion are expressed in terms of the costs of
> basic operations as reported by rtx_cost, so using an absolute constant will
> have different effects on different architectures, depending on the arbitrary
> choice of the instruction cost scale).
>
>   
>> +	fprintf (dump_file, "Decided to move dependet invariant %d\n",
>> +		 invno);
>>     
>
> dependent
>
>   
Done.
>> @@ -1154,7 +1297,8 @@ find_invariants_to_move (bool speed)
>>    /* We do not really do a good job in estimating number of registers used;
>>       we put some initial bound here to stand for induction variables etc.
>>       that we do not detect.  */
>> -  regs_used = 2;
>> +  if (! flag_ira_loop_pressure)
>> +    regs_used = 2;
>>  
>>    for (i = 0; i < n_regs; i++)
>>      {
>>     
>
> If flag_ira_loop_pressure is true, regs_used is used uninitialized in the
> following loop (you should be getting uninitialized variable warning on this).
> The loop should be guarded by !flag_ira_loop_pressure as well.
>
>   
Done. For some reasons, I did not get this warning neither during the 
build nor during the bootstrap.
>> -  new_regs = 0;
>> -  while (best_gain_for_invariant (&inv, &regs_needed, new_regs, regs_used, speed) > 0)
>> +  if (! flag_ira_loop_pressure)
>> +    new_regs[0] = regs_needed[0] = 0;
>> +  else
>>      {
>> -      set_move_mark (inv->invno);
>> -      new_regs += regs_needed;
>> +      for (i = 0; (int) i < ira_reg_class_cover_size; i++)
>> +	{
>> +	  new_regs[ira_reg_class_cover[i]] = 0;
>> +	  regs_needed[ira_reg_class_cover[i]] = 0;
>> +	}
>> +    }
>>     
>
> It should not be necessary to zero the regs_needed array.
>
>   
I've removed this.
>> +DEFPARAM (PARAM_IRA_LOOP_RESERVED_REGS,
>> +	  "ira-loop-reserved-regs",
>> +	  "The number of registers reserved for loop invariant motion",
>> +	  2, 0, 0)
>>     
>
> "The number of registers in each class kept unused by loop invariant motion"
>
> would seem like a better description.
>   
Done.  Here the new variant of the files mentioned by you.


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: Changes
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20091001/8f485afa/attachment.ksh>