Combiner fix for PR79910

On 03/15/2017 12:09 AM, Bernd Schmidt wrote:
>  I'll retest with your
> suggestion and with the bitmap creation conditional on i1 being nonnull.

Like this (also had to throw in a bitmap_empty_p). Retested as before. Ok?


Bernd

On 03/15/2017 09:00 AM, Bernd Schmidt wrote:
> On 03/15/2017 12:09 AM, Bernd Schmidt wrote:
>>  I'll retest with your
>> suggestion and with the bitmap creation conditional on i1 being nonnull.
>
> Like this (also had to throw in a bitmap_empty_p). Retested as before. Ok?
Yea, not surprised you needed the bitmap_empty_p.

OK with or without the debug counter -- your decide on whether or not to 
include it.

jeff

On 03/15/2017 11:07 AM, Jeff Law wrote:
> On 03/15/2017 09:00 AM, Bernd Schmidt wrote:
>> On 03/15/2017 12:09 AM, Bernd Schmidt wrote:
>>>  I'll retest with your
>>> suggestion and with the bitmap creation conditional on i1 being nonnull.
>>
>> Like this (also had to throw in a bitmap_empty_p). Retested as before.
>> Ok?
> Yea, not surprised you needed the bitmap_empty_p.
>
> OK with or without the debug counter -- your decide on whether or not to
> include it.
Given your note that you had planned to remove the dbg_cnt stuff, I went 
ahead and remove the dbg_cnt stuff, and installed the patch (and testcase).

Jeff

Thanks for not cc:ing me on any of this.

On Wed, Mar 15, 2017 at 04:00:21PM +0100, Bernd Schmidt wrote:
> +/* Set up a set of registers used in an insn.  Called through note_uses,
> +   arguments as described for that function.  */
> +
> +static void
> +record_used_regs (rtx *xptr, void *data)
> +{
> +  bitmap set = (bitmap)data;

Space after cast, throughout.

> +  int i, j;
> +  enum rtx_code code;
> +  const char *fmt;
> +  rtx x = *xptr;
> +
> +  /* repeat is used to turn tail-recursion into iteration since GCC
> +     can't do it when there's no return value.  */
> + repeat:

This comment is incorrect.

> +	  /* If we are about to do the last recursive call
> +	     needed at this level, change it into iteration.
> +	     This function is called enough to be worth it.  */

This function is called enough that it should not be called at all.

> +  /* For combinations that may result in two insns, we have to gather
> +     some extra information about registers used, so that we can
> +     update all relevant LOG_LINKS later.  */

Please just refuse to do the combination in such cases instead.

> +	/* See comments above where we calculate the bitmap.  */
> +	EXECUTE_IF_SET_IN_BITMAP ((bitmap)new_regs_in_i2,
> +				  LAST_VIRTUAL_REGISTER, i, iter)

Why do you need a cast here at all?

> +	  {
> +	    rtx reg = regno_reg_rtx[i];
> +	    rtx_insn *other;
> +	    for (other = NEXT_INSN (i2); other != i3; other = NEXT_INSN (other))
> +	      if (NONDEBUG_INSN_P (other)
> +		  && (reg_overlap_mentioned_p (reg, PATTERN (other))
> +		      || (CALL_P (other) && find_reg_fusage (other, USE, reg))))
> +		{
> +		  if (dump_file)
> +		    fprintf (dump_file,
> +			     "found extra use of reg %d at insn %d\n", i,
> +			     INSN_UID (other));
> +		  insn_link **plink;
> +		  for (plink = &LOG_LINKS (other);
> +		       *plink;
> +		       plink = &(*plink)->next)
> +		    {
> +		      insn_link *link = *plink;
> +		      if (link->regno == i)
> +			{
> +			  *plink = link->next;
> +			  link->next = i3links;
> +			  i3links = link;
> +			  break;
> +			}
> +		    }
> +		  break;
> +		}
> +	  }

This should be a separate function.

So, no, I'm not okay with this.  It is very expensive, it is doing open
heart surgery on combine's internal structures (in a way that may or may
not work), and all that to combine some insns in a case that should not
exist anyway.


Segher

On 03/17/2017 04:14 PM, Segher Boessenkool wrote:
> Thanks for not cc:ing me on any of this.
There's really no need for getting upset.  Bernd posted the message to 
the patches list.  That's sufficient.

>
> On Wed, Mar 15, 2017 at 04:00:21PM +0100, Bernd Schmidt wrote:
>> +/* Set up a set of registers used in an insn.  Called through note_uses,
>> +   arguments as described for that function.  */
>> +
>> +static void
>> +record_used_regs (rtx *xptr, void *data)
>> +{
>> +  bitmap set = (bitmap)data;
>
> Space after cast, throughout.
A nit.  Bernd, can you please fix this.

>
>> +  int i, j;
>> +  enum rtx_code code;
>> +  const char *fmt;
>> +  rtx x = *xptr;
>> +
>> +  /* repeat is used to turn tail-recursion into iteration since GCC
>> +     can't do it when there's no return value.  */
>> + repeat:
>
> This comment is incorrect.
Depends on the situation.  GCC's ability to turn tail recursion into 
iteration is not good.

>
>> +	  /* If we are about to do the last recursive call
>> +	     needed at this level, change it into iteration.
>> +	     This function is called enough to be worth it.  */
>
> This function is called enough that it should not be called at all.
I disagree.  We have a code gen bug. We need a solution.  Feel free to 
come up with something better.

>
>> +  /* For combinations that may result in two insns, we have to gather
>> +     some extra information about registers used, so that we can
>> +     update all relevant LOG_LINKS later.  */
>
> Please just refuse to do the combination in such cases instead.
?!? That would essentially mean we can't do 3->2 combinations.

>
>> +	/* See comments above where we calculate the bitmap.  */
>> +	EXECUTE_IF_SET_IN_BITMAP ((bitmap)new_regs_in_i2,
>> +				  LAST_VIRTUAL_REGISTER, i, iter)
>
> Why do you need a cast here at all?
>
>> +	  {
>> +	    rtx reg = regno_reg_rtx[i];
>> +	    rtx_insn *other;
>> +	    for (other = NEXT_INSN (i2); other != i3; other = NEXT_INSN (other))
>> +	      if (NONDEBUG_INSN_P (other)
>> +		  && (reg_overlap_mentioned_p (reg, PATTERN (other))
>> +		      || (CALL_P (other) && find_reg_fusage (other, USE, reg))))
>> +		{
>> +		  if (dump_file)
>> +		    fprintf (dump_file,
>> +			     "found extra use of reg %d at insn %d\n", i,
>> +			     INSN_UID (other));
>> +		  insn_link **plink;
>> +		  for (plink = &LOG_LINKS (other);
>> +		       *plink;
>> +		       plink = &(*plink)->next)
>> +		    {
>> +		      insn_link *link = *plink;
>> +		      if (link->regno == i)
>> +			{
>> +			  *plink = link->next;
>> +			  link->next = i3links;
>> +			  i3links = link;
>> +			  break;
>> +			}
>> +		    }
>> +		  break;
>> +		}
>> +	  }
>
> This should be a separate function.
Bernd, can you pull this out into its own function.

>
> So, no, I'm not okay with this.  It is very expensive, it is doing open
> heart surgery on combine's internal structures (in a way that may or may
> not work), and all that to combine some insns in a case that should not
> exist anyway.
Please don't be dramatic.  If you've got a better suggestion for how to 
fix this, be my guest.  I don't like the compile-time cost either, but I 
don't see a better solution.

Jeff