Re: ia32: peephole2

[Bernd Schmidt <bernds@cygnus.co.uk>]

> The object of the peephole2 is arbitrary rtl->rtl transformations
> with multiple insns in and multiple insns out.  In addition, a 
> SCRATCH pattern in the output template matches iff there is a 
> free register of the proper class.

I've spent some time looking at it, and there appear to be some problems.
However, some of these may be just me misunderstanding the code;  if so,
could you clarify why these points aren't an issue?

First of all, if I add this peephole to i386.md (on the branch) for testing
purposes:

(define_peephole2
  [(set (match_operand:SI 0 "register_operand" "")
	(match_operand:SI 1 "register_operand" ""))
   (set (match_operand:SI 2 "register_operand" "")
        (match_operand:SI 3 "register_operand" ""))]
  ""
  [(set (match_dup 0) (match_dup 1))
   (set (match_dup 2) (match_dup 3))]
  "")

the resulting compiler crashes when compiling divdi3.  Bogus code is
generated in peephole2_1; we have

  x1 = recog_next_insn (insn, 1, _last_insn);
  if (x1 == NULL_RTX)
    goto L3639;

and

    L3639: ATTRIBUTE_UNUSED_LABEL
      if (memory_operand (x1, SImode))
	{
	  ro[0] = x1;
	  goto L3640;
	}

which leads to a crash in memory_operand.  I'm not entirely sure how the
multi-insn recognition code will work; is there a risk of the following
happening:
  match first insn
  call recog_next_insn
  try to match second insn, but fail
  return to match first insn
If so, does this work properly, i.e. is there no risk of overwriting vital
information while trying to match the second insn (the recog_operand array
comes to mind)?

Also, before every call to one of the gen_peephole2 functions, we have
        *_last_insn = insn;
although INSN is never modified in the function, so it appears that it will
always write the original insn.  The call to recog_next_insn, however, could
have modified *_last_insn.  That doesn't appear right.

I'm also not quite sure about the code that allocates new registers.
find_free_register seems to compute a set of registers free at one specific
point in the rtl.  However, if we have a define_peephole2 for a sequence of
multiple insns, some registers may be in use at other places in the sequence
and therefore unavailable for scratch regs.  Silly example (again, only for
demonstration purposes, I know it doesn't make sense):

(define_peephole2
  [(set (match_operand 0) (match_operand 1))
   (set (match_operand 2) (match_operand 3))]
  ""
  [(set (match_dup 2) (match_dup 3))
   (set (match_scratch 4) (match_dup 1))
   (set (match_dup 0) (match_dup 4))]

If find_free_registers is used to find a register free before either of the
two insns in the source of the peephole, there is a risk of returning the
reg that contains operand 2.

If necessary, we could rip out support for multiple insns in peephole2.  The
current new_ia32 backend doesn't need it.

Bernd