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

Reply via email to