RE: [PATCH RFA] Implement register pressure directed hoist pass

["Bin Cheng" <bin dot cheng at arm dot com>]

Hi Steven,
Thanks for the comments and sorry for the delay with this message.

> -----Original Message-----
> 
> Hello,
> 
> Thanks for the update. The first look wasn't a very thorough review, so I
have
> more comments now. Sorry for that, I should have taken the time for this
the
> first time round...

It's OK.

> > -   - do rough calc of how many regs are needed in each block, and a
rough
> > -     calc of how many regs are available in each class and use that to
> > -     throttle back the code in cases where RTX_COST is minimal.
> 
> This comment still applies to LCM.

It seems this comment just express the idea using register pressure
information to direct code movement for expressions with small
RTX_COST(max_distance). But the threshold of max_distance is only
implemented for hoist in GCC, I assume this comment does not affect LCM.
Could you explain more how this comments applies to LCM?

> But then again...
> 
> > +	  while (n_regs_set-- > 0)
> > +	    {
> > +	      rtx note = find_regno_note (insn, REG_UNUSED,
> > +					  REGNO (regs_set[n_regs_set]));
> > +	      if (! note)
> > +		continue;
> > +
> > +	      mark_reg_death (XEXP (note, 0));
> > +	    }
> 
> Why not just mark all registers mentioned in REG_UNUSED notes as death,
i.e.:
> 
> for (note = REG_NOTES (insn); note; note = XEXP (note, 1))
>   if ((REG_NOTE_KIND (note) == REG_UNUSED)
>     mark_reg_death (XEXP (note, 0));
> 
> ?

I don't know the exact reason, can only guess REG_UNUSED notes are recorded
only for set registers here. Since this calculation code is a copy of
existing codes, maybe Vlad can help explain this? Thanks.

> 
> 
> > +fira-hoist-pressure
> > +Common Report Var(flag_ira_hoist_pressure) Init(-1) Use IRA based
> > +register pressure calculation in hoist optimizations.
> 
> Please add the "Optimization" marker. Why initialize to -1? The default
> initialization is 0, and if the flag is set it takes a value 1. If you
follow
> that common behavior, you can replace all occurrences of "if
> (flag_ira_hoist_pressure == 1)" with just ""if (flag_ira_hoist_pressure)".
> 

The reason is because we only want to enable this for Thumb1 originally, and
make it possible to disable it for Thumb1 on command line by specifying
"-fno-ira-hoist-pressure".

> 
> > +  /* Enable register pressure hoist when optimizing for size on
> > + Thumb1 set.  */  if (TARGET_THUMB1 && optimize_function_for_size_p
(cfun)
> > +      && flag_ira_hoist_pressure == -1)
> > +    flag_ira_hoist_pressure = 1;
> 
> One would expect this to be a win on all targets, but you probably looked
at
> this (at least Thumb2 and plain ARM). Did your patch not help there?
Otherwise,
> I'd be in favor of enabling this option by default for all targets.

>From the data I collected, it helps for Thumb1/ARM/Mips and maybe x86_64 a
little bit. It has no obvious effect on Thumb2/x86.
Since Jeff also expressed that it might be enabled for all targets, I will
collect size data for more targets. Your previous concern will also be
addressed in this way.

> 
> 
> > +  reg = SET_DEST (set);
> > +  if (GET_CODE (reg) == SUBREG)
> > +    reg = SUBREG_REG (reg);
> > +  if (MEM_P (reg))
> > +    {
> > +      *nregs = 0;
> > +      pressure_class = NO_REGS;
> > +    }
> > +  else
> > +    {
> > +      if (! REG_P (reg))
> > +	reg = NULL_RTX;
> > +      if (reg == NULL_RTX)
> > +	pressure_class = GENERAL_REGS;
> 
> gcc_assert (REG_P (reg))? If reg is not a MEM, it must be a REG or it's
not
> recorded in compute_hash_table. In fact, AFAIR reg must be a REG unless
> flag_gcse_las is set, and I don't know if that even works with code
hoisting.
> It's not a particularly well tested flag (although it's a very useful one
for
> most RISC targets, maybe you should have a look at that, too, for ARM
> variants).

Again, it is a copy from loop-invariant.c. It might be rewritten/optimized
for hoist, but should we keep it same with original copy in
loop-invariant.c?

> 
> Ciao!
> Steven

Thanks again for your review, all other comments are accepted and I will
work out an updated patch for review.