Re: [PATCH/RFC] combine: Tweak the condition of last_set invalidation

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Wed, Dec 16, 2020 at 04:49:49PM +0800, Kewen.Lin wrote:
> Currently we have the check:
> 
>       if (!insn
> 	  || (value && rsp->last_set_table_tick >= label_tick_ebb_start))
> 	rsp->last_set_invalid = 1; 
> 
> which means if we want to record some value for some reg and
> this reg got refered before in a valid scope,

If we already know it is *set* in this same extended basic block.
Possibly by the same instruction btw.

> we invalidate the
> set of reg (last_set_invalid to 1).  It avoids to find the wrong
> set for one reg reference, such as the case like:
> 
>    ... op regX  // this regX could find wrong last_set below
>    regX = ...   // if we think this set is valid
>    ... op regX

Yup, exactly.

> But because of retry's existence, the last_set_table_tick could
> be set by some later reference insns, but we see it's set due
> to retry on the set (for that reg) insn again, such as:
> 
>    insn 1
>    insn 2
> 
>    regX = ...     --> (a)
>    ... op regX    --> (b)
>    
>    insn 3
> 
>    // assume all in the same BB.
> 
> Assuming we combine 1, 2 -> 3 sucessfully and replace them as two
> (3 insns -> 2 insns),

This will delete insn 1 and write the combined result to insns 2 and 3.

> retrying from insn1 or insn2 again:

Always 2, but your point remains valid.

> it will scan insn (a) again, the below condition holds for regX:
> 
>   (value && rsp->last_set_table_tick >= label_tick_ebb_start)
> 
> it will mark this set as invalid set.  But actually the
> last_set_table_tick here is set by insn (b) before retrying, so it
> should be safe to be taken as valid set.

Yup.

> This proposal is to check whether the last_set_table safely happens
> after the current set, make the set still valid if so.

> Full SPEC2017 building shows this patch gets more sucessful combines
> from 1902208 to 1902243 (trivial though).

Do you have some example, or maybe even a testcase?  :-)

> +  /* Record the luid of the insn whose expression involving register n.  */
> +
> +  int				last_set_table_luid;

"Record the luid of the insn for which last_set_table_tick was set",
right?

> -static void update_table_tick (rtx);
> +static void update_table_tick (rtx, int);

Please remove this declaration instead, the function is not used until
after its actual definition :-)

> @@ -13243,7 +13247,21 @@ update_table_tick (rtx x)
>        for (r = regno; r < endregno; r++)
>  	{
>  	  reg_stat_type *rsp = &reg_stat[r];
> -	  rsp->last_set_table_tick = label_tick;
> +	  if (rsp->last_set_table_tick >= label_tick_ebb_start)
> +	    {
> +	      /* Later references should not have lower ticks.  */
> +	      gcc_assert (label_tick >= rsp->last_set_table_tick);

This should be obvious, but checking it won't hurt, okay.

> +	      /* Should pick up the lowest luid if the references
> +		 are in the same block.  */
> +	      if (label_tick == rsp->last_set_table_tick
> +		  && rsp->last_set_table_luid > insn_luid)
> +		rsp->last_set_table_luid = insn_luid;

Why?  Is it conservative for the check you will do later?  Please spell
this out, it is crucial!

> @@ -13359,7 +13378,10 @@ record_value_for_reg (rtx reg, rtx_insn *insn, rtx value)
>  
>    /* Mark registers that are being referenced in this value.  */
>    if (value)
> -    update_table_tick (value);
> +    {
> +      gcc_assert (insn);
> +      update_table_tick (value, DF_INSN_LUID (insn));
> +    }

Don't add that assert please.  If you really want one it should come
right at the start of the function, not 60 lines later :-)

Looks good if I understood this correctly :-)


Segher