Combiner fix for PR79910

Jeff Law law@redhat.com
Tue Mar 14 23:03:00 GMT 2017


On 03/10/2017 04:24 PM, Bernd Schmidt wrote:
> In this PR, we have a few insns involved in two instruction combinations:
>
> insn 16: set r100
> insn 27: some calculation
> insn 28: some calculation
> insn 32: using r100
> insn 33: using r100
> insn 35: some calculation
>
> Then we combine insns 27, 28 and 33, producing two output insns, As a
> result, insn 28 (i2) now obtains a use of r100. But insn 32, which is
> not involved in this combination, has the LOG_LINKS entry for that
> register, and we don't know that we need to update it. As a result, the
> second combination, involving regs 16, 32 and 35 (based on the remaining
> LOG_LINK for r100), produces incorrect code, as we don't realize there's
> a use of r100 before insn 32.
>
> The following fixes it. Bootstrapped and tested on x86_64-linux, ok (on
> all branches)?
>
>
> Bernd
>
>
> 79910.diff
>
>
> 	PR rtl-optimization/79910
> 	* combine.c (record_used_regs): New static function.
> 	(try_combine): Handle situations where there is an additional
> 	instruction between I2 and I3 which needs to have a LOG_LINK
> 	updated.
>
> 	PR rtl-optimization/79910
> 	* gcc.dg/torture/pr79910.c: New test.
What a nasty little problem.

I don't like that we have to build these bitmaps due to the compile-time 
cost.  Would it make sense to only build them when i0/i1 exist?

We don't do 4->3 combinations, just 4->2 and 3->2, so it's only the 
i2pattern where we might need to conjure up a LOG_LINK, right?


We don't do 4->3 combinations, right?  So we only have to care about 
when there's going to be an newi2pat, right (3->2 or 4->2).
We don't ever create a newi1pat (for a 4->3 combination), right?  So we 
only have to worry about testing when there's a newi2pat, right?


Do you need to handle 4->3, in which case the new bits could be on newi1pat?
>
> Index: gcc/combine.c
> ===================================================================
> --- gcc/combine.c	(revision 245685)
> +++ gcc/combine.c	(working copy)
> @@ -2559,6 +2560,57 @@ can_split_parallel_of_n_reg_sets (rtx_in
>    return true;
>  }
>

> @@ -4051,6 +4124,33 @@ try_combine (rtx_insn *i3, rtx_insn *i2,
>        return 0;
>      }
>
> +  auto_bitmap new_regs_in_i2;
> +  if (newi2pat)
> +    {
> +      /* We need to discover situations where we introduce a use of a
> +	 register into I2, where none of the existing LOG_LINKS contain
> +	 a reference to it.  This can happen if previously I3 referenced
> +	 the reg, and there is an additional use between I2 and I3.  We
> +	 must remove the LOG_LINKS entry from that additional use and
> +	 distribute it along with our own ones.  */
> +	note_uses (&newi2pat, record_used_regs, (bitmap)new_regs_in_i2);
> +	bitmap_and_compl_into (new_regs_in_i2, i2_regset);
> +	bitmap_and_compl_into (new_regs_in_i2, links_regset);
> +
> +	/* Here, we first look for situations where a hard register use
> +	   moved, and just give up.  This should happen approximately
> +	   never, and it's not worth it to deal with possibilities like
> +	   multi-word registers.  Later, when fixing up LOG_LINKS, we
> +	   deal with the case where a pseudo use moved.  */
> +	if (prev_nonnote_insn (i3) != i2)
> +	  for (unsigned r = 0; r < FIRST_PSEUDO_REGISTER; r++)
> +	    if (bitmap_bit_p (new_regs_in_i2, r))
if (prev_nonnote_insn (i3) != i2
     && bitmap_first_set_bit (new_regs_in_i2) < FIRST_PSEUDO_REGISTER)

?



Overall it seems reasonable.  If possible, let's avoid the calls to 
create the bitmaps in case where we know we'll never be creating a new 
use in i2.

I'm not wed to using bitmap_first_set_bit.  It's clearer and may be 
faster since you're not doing a ton of calls into bitmap_bit_p.  The 
bitmap_first_set_bit implementation under the hood finds the word within 
the first bitmap element that is nonzero, then uses ctzl on that to get 
its bit.

Jeff



More information about the Gcc-patches mailing list