Problem in gen_reload(), seeking an explanation

Problem in gen_reload(), seeking an explanation

[Franz Sirl]

Hi,

on PPC gcc-2.95pre I have a testcase that aborts in gen_add2_insn() called 
by gen_reload(). The problem is a insn with 3 different real registers like 
this

REG_CLASS(GENERAL_REGS) = REG_CLASS(CTR_REGS) + REG_CLASS(GENERAL_REGS)

There's no instruction on PPC that can handle these regclasses a single 
instruction add, so gen_reload() tries to use a move/add sequence. And here 
starts the problem, cause it first tries

move GENERAL_REGS,GENERAL_REGS
add GENERAL_REGS,CTR_REGS

with the add generated by gen_add2_insn(). Unfortunately gen_add2_insn() 
aborts here, cause there is no "add GENERAL_REGS,CTR_REGS" on PPC.
But there is code in gen_reload() lateron that would try the following:

move GENERAL_REGS,CTR_REGS
add GENERAL_REGS,GENERAL_REGS

And that works on PPC. Looking at the code in gen_reload() I saw that it 
makes a difference for gen_reload() if it is presented a

REG_CLASS(GENERAL_REGS) = REG_CLASS(CTR_REGS) + REG_CLASS(GENERAL_REGS)

or a

REG_CLASS(GENERAL_REGS) = REG_CLASS(GENERAL_REGS) + REG_CLASS(CTR_REGS)

The latter one is handled just fine in gen_reload().

I submitted a patch to replace the gen_add2_insn() with a general gen_rtx 
expression to avoid the abort, but Jeff doesn't like it :-(.

So my questions are:
- is gen_reload() supposed to handle such an add with 3 regs/2 regclasses?
- should swapping the regclasses change the behaviour at all?
- if gen_reload() is not able to handle this, what is the rule for that? 
Like: "Never enter here with different regclasses"?

I would really appreciate some information on that issue.

Thanks,
Franz.

Re: Problem in gen_reload(), seeking an explanation

[David Edelsohn]

	We need to use the gen_rtx_PLUS / gen_rtx_SET combination which
will work.  The comments say:

      /* Since constraint checking is strict, commutativity won't be
         checked, so we need to do that here to avoid spurious failure
         if the add instruction is two-address and the second operand
         of the add is the same as the reload reg, which is frequently
         the case.  If the insn would be A = B + A, rearrange it so
         it will be A = A + B as constrain_operands expects.  */

We are trying to do GENERAL_REGS = CTR_REGS + GENERAL_REGS which should be
converted to GENERAL_REGS = GENERAL_REGS + CTR_REGS (which will succeed)
by the commutative transformation.  Why isn't this succeeding?

David

Re: Problem in gen_reload(), seeking an explanation

[Jeffrey A Law]

  In message < 4.2.0.56.19990616181739.039cabd0@mail.lauterbach.com >you write:
  > Hi,
  > 
  > on PPC gcc-2.95pre I have a testcase that aborts in gen_add2_insn() called 
  > by gen_reload(). The problem is a insn with 3 different real registers like
  >  
  > this
  > 
  > REG_CLASS(GENERAL_REGS) = REG_CLASS(CTR_REGS) + REG_CLASS(GENERAL_REGS)
  > 
  > There's no instruction on PPC that can handle these regclasses a single 
  > instruction add, so gen_reload() tries to use a move/add sequence. And here
  >  
  > starts the problem, cause it first tries
  > 
  > move GENERAL_REGS,GENERAL_REGS
  > add GENERAL_REGS,CTR_REGS
  > 
  > with the add generated by gen_add2_insn(). Unfortunately gen_add2_insn() 
  > aborts here, cause there is no "add GENERAL_REGS,CTR_REGS" on PPC.
  > But there is code in gen_reload() lateron that would try the following:
What I have maintained all along is that this should have been handled before
we called gen_add2_insn.  

If you look earlier in gen_reload we have support for rearranging the operands
of an add instruction to avoid this kind of problem.  You need to determine
why that code is not shuffling the operands so that the add looks like
GPRx = GPRx + CTR


  > I submitted a patch to replace the gen_add2_insn() with a general gen_rtx 
  > expression to avoid the abort, but Jeff doesn't like it :-(.
I don't like it because it's the wrong solution.  You're fixing the symptom
(gen_add2_insn aborting), not the problem (we never should have called 
gen_add2_insn with bogus operands).



  > So my questions are:
  > - is gen_reload() supposed to handle such an add with 3 regs/2 regclasses?
Yes.  If it does not, then that would be the real bug IMHO.  If you don't
have a 3 operand add, then you have to generate a move; add sequence.   It
may be the case that we do not perform the necessary operand shuffling when
we create the move;add sequence. 

jeff

Re: Problem in gen_reload(), seeking an explanation

[Franz Sirl]

At 13:24 21.06.99 , Jeffrey A Law wrote:

>   In message < 4.2.0.56.19990616181739.039cabd0@mail.lauterbach.com >you write:
>   > Hi,
>   >
>   > on PPC gcc-2.95pre I have a testcase that aborts in gen_add2_insn() 
> called
>   > by gen_reload(). The problem is a insn with 3 different real 
> registers like
>   >
>   > this
>   >
>   > REG_CLASS(GENERAL_REGS) = REG_CLASS(CTR_REGS) + REG_CLASS(GENERAL_REGS)
>   >
>   > There's no instruction on PPC that can handle these regclasses a single
>   > instruction add, so gen_reload() tries to use a move/add sequence. 
> And here
>   >
>   > starts the problem, cause it first tries
>   >
>   > move GENERAL_REGS,GENERAL_REGS
>   > add GENERAL_REGS,CTR_REGS
>   >
>   > with the add generated by gen_add2_insn(). Unfortunately gen_add2_insn()
>   > aborts here, cause there is no "add GENERAL_REGS,CTR_REGS" on PPC.
>   > But there is code in gen_reload() lateron that would try the following:
>What I have maintained all along is that this should have been handled before
>we called gen_add2_insn.
>
>If you look earlier in gen_reload we have support for rearranging the operands
>of an add instruction to avoid this kind of problem.  You need to determine
>why that code is not shuffling the operands so that the add looks like
>GPRx = GPRx + CTR

The code is not shuffling operands cause it is only special-casing 
pseudo-registers, but we enter here with 2 real registers. Something like 
below works too:

--- reload1.c   Thu Jun 17 08:24:51 1999
+++ reload1.c.fix2      Sun Jun 13 08:41:31 1999
@@ -7821,10 +7821,16 @@ gen_reload (out, in, opnum, type)

        if (CONSTANT_P (op1) || GET_CODE (op1) == MEM || GET_CODE (op1) == 
SUBREG
           || (GET_CODE (op1) == REG
-             && REGNO (op1) >= FIRST_PSEUDO_REGISTER))
+             && REGNO (op1) >= FIRST_PSEUDO_REGISTER)
+         || (GET_CODE (op0) == REG
+             && REGNO (op0) < FIRST_PSEUDO_REGISTER
+             && REGNO_REG_CLASS (REGNO (op0)) == GENERAL_REGS))
+             && GET_CODE (op1) == REG
+             && REGNO (op1) < FIRST_PSEUDO_REGISTER
+             && REGNO_REG_CLASS (REGNO (op1)) != GENERAL_REGS))
         tem = op0, op0 = op1, op1 = tem;

       gen_reload (out, op0, opnum, type);

What I'm not quite sure about is if and how we can enter here with float 
operands.


>   > I submitted a patch to replace the gen_add2_insn() with a general 
> gen_rtx
>   > expression to avoid the abort, but Jeff doesn't like it :-(.
>I don't like it because it's the wrong solution.  You're fixing the symptom
>(gen_add2_insn aborting), not the problem (we never should have called
>gen_add2_insn with bogus operands).

Jeff, I only wanted you to elaborate a bit on _why_ it's wrong. Why is a 
general gen_rtx_SET ok for a 3-operand add, but not for a 2-operand add? 
Especially as the code immediately following is the same for both. I've 
heard no argument on that so far. Changing the swap behaviour was in fact 
my first approach, but looking at the rest of gen_reload the gen_rtx_SET 
looked a lot more natural and general.

>   > So my questions are:
>   > - is gen_reload() supposed to handle such an add with 3 regs/2 
> regclasses?
>Yes.  If it does not, then that would be the real bug IMHO.  If you don't
>have a 3 operand add, then you have to generate a move; add sequence.   It
>may be the case that we do not perform the necessary operand shuffling when
>we create the move;add sequence.

Ok.

Franz.

Re: Problem in gen_reload(), seeking an explanation

[Joern Rennecke]

>            || (GET_CODE (op1) == REG
> -             && REGNO (op1) >= FIRST_PSEUDO_REGISTER))
> +             && REGNO (op1) >= FIRST_PSEUDO_REGISTER)
> +         || (GET_CODE (op0) == REG
> +             && REGNO (op0) < FIRST_PSEUDO_REGISTER
> +             && REGNO_REG_CLASS (REGNO (op0)) == GENERAL_REGS))
> +             && GET_CODE (op1) == REG
> +             && REGNO (op1) < FIRST_PSEUDO_REGISTER
> +             && REGNO_REG_CLASS (REGNO (op1)) != GENERAL_REGS))

REGNO_REG_CLASS will always give you a smallest register class that a
register belongs to.  So you might get a subclass of GENERAL_REGS.
Examples are the various letter-named one-register-classes on x86,
M16_REGS on mips, R0_REGS on SH.

So it's better to say:

TEST_HARD_REG_BIT (reg_class_contents[GENERAL_REGS], REGNO (op0))

but that will still not handle the case properly when an add can
operate on some register class that is not a subset of GENERAL_REGS.
I don't know if we have any target where this is an issue, though.

Re: Problem in gen_reload(), seeking an explanation

[Franz Sirl]

At 16:55 21.06.99 , Joern Rennecke wrote:
> >            || (GET_CODE (op1) == REG
> > -             && REGNO (op1) >= FIRST_PSEUDO_REGISTER))
> > +             && REGNO (op1) >= FIRST_PSEUDO_REGISTER)
> > +         || (GET_CODE (op0) == REG
> > +             && REGNO (op0) < FIRST_PSEUDO_REGISTER
> > +             && REGNO_REG_CLASS (REGNO (op0)) == GENERAL_REGS))
> > +             && GET_CODE (op1) == REG
> > +             && REGNO (op1) < FIRST_PSEUDO_REGISTER
> > +             && REGNO_REG_CLASS (REGNO (op1)) != GENERAL_REGS))
>
>REGNO_REG_CLASS will always give you a smallest register class that a
>register belongs to.  So you might get a subclass of GENERAL_REGS.
>Examples are the various letter-named one-register-classes on x86,
>M16_REGS on mips, R0_REGS on SH.
>
>So it's better to say:
>
>TEST_HARD_REG_BIT (reg_class_contents[GENERAL_REGS], REGNO (op0))

OK, understood. Do I have to care about a FLOAT_REGS = FLOAT_REGS + 
GENERAL_REGS case here? Just to make sure I'm not breaking anything.

>but that will still not handle the case properly when an add can
>operate on some register class that is not a subset of GENERAL_REGS.
>I don't know if we have any target where this is an issue, though.

Uhm, such an argument would probably point to a solution with 
gen_rtx_SET(gen_rtx_PLUS()) instead of gen_add2_insn for insn validation 
again? Otherwise you would have to put a lot of checking into the swap 
decision and probably need a new backend macro.

Franz.

Re: Problem in gen_reload(), seeking an explanation

[Joern Rennecke]

> OK, understood. Do I have to care about a FLOAT_REGS = FLOAT_REGS + 
> GENERAL_REGS case here? Just to make sure I'm not breaking anything.

reload shouldn't crate floating point adds, if that's what you mean.

> >but that will still not handle the case properly when an add can
> >operate on some register class that is not a subset of GENERAL_REGS.
> >I don't know if we have any target where this is an issue, though.
> 
> Uhm, such an argument would probably point to a solution with 
> gen_rtx_SET(gen_rtx_PLUS()) instead of gen_add2_insn for insn validation 
> again? Otherwise you would have to put a lot of checking into the swap 
> decision and probably need a new backend macro.

We could also do the swap if the first operand of the add doesn't satisfies
the predicate for operand 1 of the addsi3 pattern / expander, but the
second operand of the add does.  As long as the predicates are defined
sensible, this will help.

Re: Problem in gen_reload(), seeking an explanation

[Franz Sirl]

Am Mon, 21 Jun 1999 schrieb Joern Rennecke:
>> OK, understood. Do I have to care about a FLOAT_REGS = FLOAT_REGS + 
>> GENERAL_REGS case here? Just to make sure I'm not breaking anything.
>
>reload shouldn't crate floating point adds, if that's what you mean.
>
>> >but that will still not handle the case properly when an add can
>> >operate on some register class that is not a subset of GENERAL_REGS.
>> >I don't know if we have any target where this is an issue, though.
>> 
>> Uhm, such an argument would probably point to a solution with 
>> gen_rtx_SET(gen_rtx_PLUS()) instead of gen_add2_insn for insn validation 
>> again? Otherwise you would have to put a lot of checking into the swap 
>> decision and probably need a new backend macro.
>
>We could also do the swap if the first operand of the add doesn't satisfies
>the predicate for operand 1 of the addsi3 pattern / expander, but the
>second operand of the add does.  As long as the predicates are defined
>sensible, this will help.

Hmm, wouldn't addsi3 abort() the same way as addsi2 does now? addsi2
currently aborts before we can try the move/add sequence a 2nd time with the
operands swapped.

Franz.

Re: Problem in gen_reload(), seeking an explanation

[Joern Rennecke]

> Hmm, wouldn't addsi3 abort() the same way as addsi2 does now? addsi2
> currently aborts before we can try the move/add sequence a 2nd time with the
> operands swapped.

I'm not proposing to use addsi3 itself, but just using its operand predicates.
Or more exactly, you index add_optab->handlers with the mode of the add,
and if the insn code you get is not CODE_FOR_nothing, you call
(*insn_operand_predicate[icode][1])

Re: Problem in gen_reload(), seeking an explanation

[Franz Sirl]

Am Mon, 21 Jun 1999 schrieb Joern Rennecke:
>> Hmm, wouldn't addsi3 abort() the same way as addsi2 does now? addsi2
>> currently aborts before we can try the move/add sequence a 2nd time with the
>> operands swapped.
>
>I'm not proposing to use addsi3 itself, but just using its operand predicates.
>Or more exactly, you index add_optab->handlers with the mode of the add,
>and if the insn code you get is not CODE_FOR_nothing, you call
>(*insn_operand_predicate[icode][1])

Hmm, something like this?

--- reload1.c   1999/05/08 01:34:55     1.145
+++ reload1.c   1999/06/21 20:30:19
@@ -7819,9 +7819,13 @@ gen_reload (out, in, opnum, type)
         DEFINE_PEEPHOLE should be specified that recognizes the sequence
         we emit below.  */

+      code = (int) add_optab->handlers[(int) GET_MODE (out)].insn_code;
+
       if (CONSTANT_P (op1) || GET_CODE (op1) == MEM || GET_CODE (op1) == SUBREG
          || (GET_CODE (op1) == REG
-             && REGNO (op1) >= FIRST_PSEUDO_REGISTER))
+             && REGNO (op1) >= FIRST_PSEUDO_REGISTER)
+         || (code != CODE_FOR_nothing
+             && ! (*insn_operand_predicate[code][2]) (op1, insn_operand_mode[code][2])))
        tem = op0, op0 = op1, op1 = tem;

       gen_reload (out, op0, opnum, type);

Actually I believe this solution is way better than the regclass approach. Do I
have to guard this with a (GET_CODE (op1) == REG && REGNO (op1) <
FIRST_PSEUDO_REGISTER)?

Franz.

Re: Problem in gen_reload(), seeking an explanation

[Joern Rennecke]

> Hmm, something like this?
> 
> --- reload1.c   1999/05/08 01:34:55     1.145
> +++ reload1.c   1999/06/21 20:30:19
> @@ -7819,9 +7819,13 @@ gen_reload (out, in, opnum, type)
>          DEFINE_PEEPHOLE should be specified that recognizes the sequence
>          we emit below.  */
> 
> +      code = (int) add_optab->handlers[(int) GET_MODE (out)].insn_code;
> +
>        if (CONSTANT_P (op1) || GET_CODE (op1) == MEM || GET_CODE (op1) == SUBREG
>           || (GET_CODE (op1) == REG
> -             && REGNO (op1) >= FIRST_PSEUDO_REGISTER))
> +             && REGNO (op1) >= FIRST_PSEUDO_REGISTER)
> +         || (code != CODE_FOR_nothing
> +             && ! (*insn_operand_predicate[code][2]) (op1, insn_operand_mode[code][2])))
>         tem = op0, op0 = op1, op1 = tem;
> 
>        gen_reload (out, op0, opnum, type);

Yes.  The comment above the patch would have to be updated, of course.

> Actually I believe this solution is way better than the regclass approach. Do I
> have to guard this with a (GET_CODE (op1) == REG && REGNO (op1) <
> FIRST_PSEUDO_REGISTER)?

No, that shouldn't be necessary.  If the predicate returns false, the
add can't be recognized.

Re: Problem in gen_reload(), seeking an explanation

[Jeffrey A Law]

  In message < 4.2.0.56.19990621135045.036f9220@mail.lauterbach.com >you write:
  > The code is not shuffling operands cause it is only special-casing 
  > pseudo-registers, but we enter here with 2 real registers. Something like 
  > below works too:
Not exactly what I had in mind.  But better.

If you look even earlier, you will find code that looks like this:

      if (op0 != XEXP (in, 0) || op1 != XEXP (in, 1))
        in = gen_rtx_PLUS (GET_MODE (in), op0, op1);

      insn = emit_insn (gen_rtx_SET (VOIDmode, out, in));
      code = recog_memoized (insn);

      if (code >= 0)
        {
          extract_insn (insn);
          /* We want constrain operands to treat this insn strictly in
             its validity determination, i.e., the way it would after reload
             has completed.  */
          if (constrain_operands (1))
            return insn;
        }

      delete_insns_since (last);

We can do the same thing in the later hunk of code.  ie, try it one way, if
that does not work, swap the operands and try again.  Something like this

      gen_reload (out, op0, opnum, type);

      /* If OP0 and OP1 are the same, we can use OUT for OP1.
         This fixes a problem on the 32K where the stack pointer cannot
         be used as an operand of an add insn.  */

      if (rtx_equal_p (op0, op1))
        op1 = out;

      insn = emit_insn (gen_rtx_SET ...)

      /* If that failed, copy the address register to the reload register.
         Then add the constant to the reload register.  */

      code = recog_memoized (insn);
      [ ... check that the operands match the constraints... ]
      if constraints fail, call delete_all_insns_since (last), swap
      op0 and op1 and try again.  If it still fails delete the insns and
      go on to the next method of generating the reload.



  > What I'm not quite sure about is if and how we can enter here with float 
  > operands.
I do not think in any way that matters -- most of the complexity in this code
is dealing with addresses and frame pointer elimination on 2 address machines.

jeff

Re: Problem in gen_reload(), seeking an explanation

[Jeffrey A Law]

  In message < 99062122402601.00461@ns1102.munich.netsurf.de >you write:
  > Am Mon, 21 Jun 1999 schrieb Joern Rennecke:
  > >> Hmm, wouldn't addsi3 abort() the same way as addsi2 does now? addsi2
  > >> currently aborts before we can try the move/add sequence a 2nd time with
  >  the
  > >> operands swapped.
  > >
  > >I'm not proposing to use addsi3 itself, but just using its operand predica
  > tes.
  > >Or more exactly, you index add_optab->handlers with the mode of the add,
  > >and if the insn code you get is not CODE_FOR_nothing, you call
  > >(*insn_operand_predicate[icode][1])
  > 
  > Hmm, something like this?
[ ... ]
Much better :-)  No mucking around with regclasses and generally cleaner.

Basically the updated patch says "if op1 is not a suitable operand for an
add instruction, then try to reload it into a suitable add operand".  Which
is what we really want to happen.

ie, we can reload either OP0 or OP1 into OUT.  We want to reload the one that
is not a valid add operand.  If neither is a valid add operand, then, well
we've got a real problem :-)  I do not think we need to worry about hard vs
pseudo regs here for this case

Did the revised patch fix your problem?

jeff

Re: Problem in gen_reload(), seeking an explanation

[Franz Sirl]

At 06:59 22.06.99 , Jeffrey A Law wrote:
>   In message < 4.2.0.56.19990621135045.036f9220@mail.lauterbach.com >you write:
>   > The code is not shuffling operands cause it is only special-casing
>   > pseudo-registers, but we enter here with 2 real registers. Something 
> like
>   > below works too:
>Not exactly what I had in mind.  But better.
>
>If you look even earlier, you will find code that looks like this:
>
>       if (op0 != XEXP (in, 0) || op1 != XEXP (in, 1))
>         in = gen_rtx_PLUS (GET_MODE (in), op0, op1);
>
>       insn = emit_insn (gen_rtx_SET (VOIDmode, out, in));
>       code = recog_memoized (insn);
>
>       if (code >= 0)
>         {
>           extract_insn (insn);
>           /* We want constrain operands to treat this insn strictly in
>              its validity determination, i.e., the way it would after reload
>              has completed.  */
>           if (constrain_operands (1))
>             return insn;
>         }
>
>       delete_insns_since (last);
>
>We can do the same thing in the later hunk of code.  ie, try it one way, if
>that does not work, swap the operands and try again.  Something like this
>
>       gen_reload (out, op0, opnum, type);
>
>       /* If OP0 and OP1 are the same, we can use OUT for OP1.
>          This fixes a problem on the 32K where the stack pointer cannot
>          be used as an operand of an add insn.  */
>
>       if (rtx_equal_p (op0, op1))
>         op1 = out;
>
>       insn = emit_insn (gen_rtx_SET ...)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
You are kidding, aren't you? That's exactly what my first patch did.


>       /* If that failed, copy the address register to the reload register.
>          Then add the constant to the reload register.  */
>
>       code = recog_memoized (insn);
>       [ ... check that the operands match the constraints... ]
>       if constraints fail, call delete_all_insns_since (last), swap
>       op0 and op1 and try again.  If it still fails delete the insns and
>       go on to the next method of generating the reload.

Still the same way as I suggested in my first patch.

Now, why didn't you accept my first patch?

Franz.

Re: Problem in gen_reload(), seeking an explanation

[Franz Sirl]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

At 07:11 22.06.99 , Jeffrey A Law wrote:
>   In message < 99062122402601.00461@ns1102.munich.netsurf.de >you write:
>   > Am Mon, 21 Jun 1999 schrieb Joern Rennecke:
>   > >> Hmm, wouldn't addsi3 abort() the same way as addsi2 does now? addsi2
>   > >> currently aborts before we can try the move/add sequence a 2nd 
> time with
>   >  the
>   > >> operands swapped.
>   > >
>   > >I'm not proposing to use addsi3 itself, but just using its operand 
> predica
>   > tes.
>   > >Or more exactly, you index add_optab->handlers with the mode of the add,
>   > >and if the insn code you get is not CODE_FOR_nothing, you call
>   > >(*insn_operand_predicate[icode][1])
>   >
>   > Hmm, something like this?
>[ ... ]
>Much better :-)  No mucking around with regclasses and generally cleaner.
>
>Basically the updated patch says "if op1 is not a suitable operand for an
>add instruction, then try to reload it into a suitable add operand".  Which
>is what we really want to happen.
>
>ie, we can reload either OP0 or OP1 into OUT.  We want to reload the one that
>is not a valid add operand.  If neither is a valid add operand, then, well
>we've got a real problem :-)  I do not think we need to worry about hard vs
>pseudo regs here for this case
>
>Did the revised patch fix your problem?

Yes, the problem is fixed with this patch :-). No testsuite regressions. 
This was a difficult one. What should I write in the comment Jörn requested?

Franz.

Problem in gen_reload(), seeking an explanation

[Franz Sirl]

Hi,

on PPC gcc-2.95pre I have a testcase that aborts in gen_add2_insn() called 
by gen_reload(). The problem is a insn with 3 different real registers like 
this

REG_CLASS(GENERAL_REGS) = REG_CLASS(CTR_REGS) + REG_CLASS(GENERAL_REGS)

There's no instruction on PPC that can handle these regclasses a single 
instruction add, so gen_reload() tries to use a move/add sequence. And here 
starts the problem, cause it first tries

move GENERAL_REGS,GENERAL_REGS
add GENERAL_REGS,CTR_REGS

with the add generated by gen_add2_insn(). Unfortunately gen_add2_insn() 
aborts here, cause there is no "add GENERAL_REGS,CTR_REGS" on PPC.
But there is code in gen_reload() lateron that would try the following:

move GENERAL_REGS,CTR_REGS
add GENERAL_REGS,GENERAL_REGS

And that works on PPC. Looking at the code in gen_reload() I saw that it 
makes a difference for gen_reload() if it is presented a

REG_CLASS(GENERAL_REGS) = REG_CLASS(CTR_REGS) + REG_CLASS(GENERAL_REGS)

or a

REG_CLASS(GENERAL_REGS) = REG_CLASS(GENERAL_REGS) + REG_CLASS(CTR_REGS)

The latter one is handled just fine in gen_reload().

I submitted a patch to replace the gen_add2_insn() with a general gen_rtx 
expression to avoid the abort, but Jeff doesn't like it :-(.

So my questions are:
- is gen_reload() supposed to handle such an add with 3 regs/2 regclasses?
- should swapping the regclasses change the behaviour at all?
- if gen_reload() is not able to handle this, what is the rule for that? 
Like: "Never enter here with different regclasses"?

I would really appreciate some information on that issue.

Thanks,
Franz.

Re: Problem in gen_reload(), seeking an explanation

[David Edelsohn]

	We need to use the gen_rtx_PLUS / gen_rtx_SET combination which
will work.  The comments say:

      /* Since constraint checking is strict, commutativity won't be
         checked, so we need to do that here to avoid spurious failure
         if the add instruction is two-address and the second operand
         of the add is the same as the reload reg, which is frequently
         the case.  If the insn would be A = B + A, rearrange it so
         it will be A = A + B as constrain_operands expects.  */

We are trying to do GENERAL_REGS = CTR_REGS + GENERAL_REGS which should be
converted to GENERAL_REGS = GENERAL_REGS + CTR_REGS (which will succeed)
by the commutative transformation.  Why isn't this succeeding?

David

Re: Problem in gen_reload(), seeking an explanation

[Franz Sirl]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

At 07:11 22.06.99 , Jeffrey A Law wrote:
>   In message < 99062122402601.00461@ns1102.munich.netsurf.de >you write:
>   > Am Mon, 21 Jun 1999 schrieb Joern Rennecke:
>   > >> Hmm, wouldn't addsi3 abort() the same way as addsi2 does now? addsi2
>   > >> currently aborts before we can try the move/add sequence a 2nd 
> time with
>   >  the
>   > >> operands swapped.
>   > >
>   > >I'm not proposing to use addsi3 itself, but just using its operand 
> predica
>   > tes.
>   > >Or more exactly, you index add_optab->handlers with the mode of the add,
>   > >and if the insn code you get is not CODE_FOR_nothing, you call
>   > >(*insn_operand_predicate[icode][1])
>   >
>   > Hmm, something like this?
>[ ... ]
>Much better :-)  No mucking around with regclasses and generally cleaner.
>
>Basically the updated patch says "if op1 is not a suitable operand for an
>add instruction, then try to reload it into a suitable add operand".  Which
>is what we really want to happen.
>
>ie, we can reload either OP0 or OP1 into OUT.  We want to reload the one that
>is not a valid add operand.  If neither is a valid add operand, then, well
>we've got a real problem :-)  I do not think we need to worry about hard vs
>pseudo regs here for this case
>
>Did the revised patch fix your problem?

Yes, the problem is fixed with this patch :-). No testsuite regressions. 
This was a difficult one. What should I write in the comment Jörn requested?

Franz.

Re: Problem in gen_reload(), seeking an explanation

[Jeffrey A Law]

  In message < 4.2.0.56.19990621135045.036f9220@mail.lauterbach.com >you write:
  > The code is not shuffling operands cause it is only special-casing 
  > pseudo-registers, but we enter here with 2 real registers. Something like 
  > below works too:
Not exactly what I had in mind.  But better.

If you look even earlier, you will find code that looks like this:

      if (op0 != XEXP (in, 0) || op1 != XEXP (in, 1))
        in = gen_rtx_PLUS (GET_MODE (in), op0, op1);

      insn = emit_insn (gen_rtx_SET (VOIDmode, out, in));
      code = recog_memoized (insn);

      if (code >= 0)
        {
          extract_insn (insn);
          /* We want constrain operands to treat this insn strictly in
             its validity determination, i.e., the way it would after reload
             has completed.  */
          if (constrain_operands (1))
            return insn;
        }

      delete_insns_since (last);

We can do the same thing in the later hunk of code.  ie, try it one way, if
that does not work, swap the operands and try again.  Something like this

      gen_reload (out, op0, opnum, type);

      /* If OP0 and OP1 are the same, we can use OUT for OP1.
         This fixes a problem on the 32K where the stack pointer cannot
         be used as an operand of an add insn.  */

      if (rtx_equal_p (op0, op1))
        op1 = out;

      insn = emit_insn (gen_rtx_SET ...)

      /* If that failed, copy the address register to the reload register.
         Then add the constant to the reload register.  */

      code = recog_memoized (insn);
      [ ... check that the operands match the constraints... ]
      if constraints fail, call delete_all_insns_since (last), swap
      op0 and op1 and try again.  If it still fails delete the insns and
      go on to the next method of generating the reload.



  > What I'm not quite sure about is if and how we can enter here with float 
  > operands.
I do not think in any way that matters -- most of the complexity in this code
is dealing with addresses and frame pointer elimination on 2 address machines.

jeff

Re: Problem in gen_reload(), seeking an explanation

[Franz Sirl]

At 16:55 21.06.99 , Joern Rennecke wrote:
> >            || (GET_CODE (op1) == REG
> > -             && REGNO (op1) >= FIRST_PSEUDO_REGISTER))
> > +             && REGNO (op1) >= FIRST_PSEUDO_REGISTER)
> > +         || (GET_CODE (op0) == REG
> > +             && REGNO (op0) < FIRST_PSEUDO_REGISTER
> > +             && REGNO_REG_CLASS (REGNO (op0)) == GENERAL_REGS))
> > +             && GET_CODE (op1) == REG
> > +             && REGNO (op1) < FIRST_PSEUDO_REGISTER
> > +             && REGNO_REG_CLASS (REGNO (op1)) != GENERAL_REGS))
>
>REGNO_REG_CLASS will always give you a smallest register class that a
>register belongs to.  So you might get a subclass of GENERAL_REGS.
>Examples are the various letter-named one-register-classes on x86,
>M16_REGS on mips, R0_REGS on SH.
>
>So it's better to say:
>
>TEST_HARD_REG_BIT (reg_class_contents[GENERAL_REGS], REGNO (op0))

OK, understood. Do I have to care about a FLOAT_REGS = FLOAT_REGS + 
GENERAL_REGS case here? Just to make sure I'm not breaking anything.

>but that will still not handle the case properly when an add can
>operate on some register class that is not a subset of GENERAL_REGS.
>I don't know if we have any target where this is an issue, though.

Uhm, such an argument would probably point to a solution with 
gen_rtx_SET(gen_rtx_PLUS()) instead of gen_add2_insn for insn validation 
again? Otherwise you would have to put a lot of checking into the swap 
decision and probably need a new backend macro.

Franz.

Re: Problem in gen_reload(), seeking an explanation

[Franz Sirl]

At 06:59 22.06.99 , Jeffrey A Law wrote:
>   In message < 4.2.0.56.19990621135045.036f9220@mail.lauterbach.com >you write:
>   > The code is not shuffling operands cause it is only special-casing
>   > pseudo-registers, but we enter here with 2 real registers. Something 
> like
>   > below works too:
>Not exactly what I had in mind.  But better.
>
>If you look even earlier, you will find code that looks like this:
>
>       if (op0 != XEXP (in, 0) || op1 != XEXP (in, 1))
>         in = gen_rtx_PLUS (GET_MODE (in), op0, op1);
>
>       insn = emit_insn (gen_rtx_SET (VOIDmode, out, in));
>       code = recog_memoized (insn);
>
>       if (code >= 0)
>         {
>           extract_insn (insn);
>           /* We want constrain operands to treat this insn strictly in
>              its validity determination, i.e., the way it would after reload
>              has completed.  */
>           if (constrain_operands (1))
>             return insn;
>         }
>
>       delete_insns_since (last);
>
>We can do the same thing in the later hunk of code.  ie, try it one way, if
>that does not work, swap the operands and try again.  Something like this
>
>       gen_reload (out, op0, opnum, type);
>
>       /* If OP0 and OP1 are the same, we can use OUT for OP1.
>          This fixes a problem on the 32K where the stack pointer cannot
>          be used as an operand of an add insn.  */
>
>       if (rtx_equal_p (op0, op1))
>         op1 = out;
>
>       insn = emit_insn (gen_rtx_SET ...)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
You are kidding, aren't you? That's exactly what my first patch did.


>       /* If that failed, copy the address register to the reload register.
>          Then add the constant to the reload register.  */
>
>       code = recog_memoized (insn);
>       [ ... check that the operands match the constraints... ]
>       if constraints fail, call delete_all_insns_since (last), swap
>       op0 and op1 and try again.  If it still fails delete the insns and
>       go on to the next method of generating the reload.

Still the same way as I suggested in my first patch.

Now, why didn't you accept my first patch?

Franz.

Re: Problem in gen_reload(), seeking an explanation

[Joern Rennecke]

>            || (GET_CODE (op1) == REG
> -             && REGNO (op1) >= FIRST_PSEUDO_REGISTER))
> +             && REGNO (op1) >= FIRST_PSEUDO_REGISTER)
> +         || (GET_CODE (op0) == REG
> +             && REGNO (op0) < FIRST_PSEUDO_REGISTER
> +             && REGNO_REG_CLASS (REGNO (op0)) == GENERAL_REGS))
> +             && GET_CODE (op1) == REG
> +             && REGNO (op1) < FIRST_PSEUDO_REGISTER
> +             && REGNO_REG_CLASS (REGNO (op1)) != GENERAL_REGS))

REGNO_REG_CLASS will always give you a smallest register class that a
register belongs to.  So you might get a subclass of GENERAL_REGS.
Examples are the various letter-named one-register-classes on x86,
M16_REGS on mips, R0_REGS on SH.

So it's better to say:

TEST_HARD_REG_BIT (reg_class_contents[GENERAL_REGS], REGNO (op0))

but that will still not handle the case properly when an add can
operate on some register class that is not a subset of GENERAL_REGS.
I don't know if we have any target where this is an issue, though.

Re: Problem in gen_reload(), seeking an explanation

[Joern Rennecke]

> OK, understood. Do I have to care about a FLOAT_REGS = FLOAT_REGS + 
> GENERAL_REGS case here? Just to make sure I'm not breaking anything.

reload shouldn't crate floating point adds, if that's what you mean.

> >but that will still not handle the case properly when an add can
> >operate on some register class that is not a subset of GENERAL_REGS.
> >I don't know if we have any target where this is an issue, though.
> 
> Uhm, such an argument would probably point to a solution with 
> gen_rtx_SET(gen_rtx_PLUS()) instead of gen_add2_insn for insn validation 
> again? Otherwise you would have to put a lot of checking into the swap 
> decision and probably need a new backend macro.

We could also do the swap if the first operand of the add doesn't satisfies
the predicate for operand 1 of the addsi3 pattern / expander, but the
second operand of the add does.  As long as the predicates are defined
sensible, this will help.

Re: Problem in gen_reload(), seeking an explanation

[Franz Sirl]

At 13:24 21.06.99 , Jeffrey A Law wrote:

>   In message < 4.2.0.56.19990616181739.039cabd0@mail.lauterbach.com >you write:
>   > Hi,
>   >
>   > on PPC gcc-2.95pre I have a testcase that aborts in gen_add2_insn() 
> called
>   > by gen_reload(). The problem is a insn with 3 different real 
> registers like
>   >
>   > this
>   >
>   > REG_CLASS(GENERAL_REGS) = REG_CLASS(CTR_REGS) + REG_CLASS(GENERAL_REGS)
>   >
>   > There's no instruction on PPC that can handle these regclasses a single
>   > instruction add, so gen_reload() tries to use a move/add sequence. 
> And here
>   >
>   > starts the problem, cause it first tries
>   >
>   > move GENERAL_REGS,GENERAL_REGS
>   > add GENERAL_REGS,CTR_REGS
>   >
>   > with the add generated by gen_add2_insn(). Unfortunately gen_add2_insn()
>   > aborts here, cause there is no "add GENERAL_REGS,CTR_REGS" on PPC.
>   > But there is code in gen_reload() lateron that would try the following:
>What I have maintained all along is that this should have been handled before
>we called gen_add2_insn.
>
>If you look earlier in gen_reload we have support for rearranging the operands
>of an add instruction to avoid this kind of problem.  You need to determine
>why that code is not shuffling the operands so that the add looks like
>GPRx = GPRx + CTR

The code is not shuffling operands cause it is only special-casing 
pseudo-registers, but we enter here with 2 real registers. Something like 
below works too:

--- reload1.c   Thu Jun 17 08:24:51 1999
+++ reload1.c.fix2      Sun Jun 13 08:41:31 1999
@@ -7821,10 +7821,16 @@ gen_reload (out, in, opnum, type)

        if (CONSTANT_P (op1) || GET_CODE (op1) == MEM || GET_CODE (op1) == 
SUBREG
           || (GET_CODE (op1) == REG
-             && REGNO (op1) >= FIRST_PSEUDO_REGISTER))
+             && REGNO (op1) >= FIRST_PSEUDO_REGISTER)
+         || (GET_CODE (op0) == REG
+             && REGNO (op0) < FIRST_PSEUDO_REGISTER
+             && REGNO_REG_CLASS (REGNO (op0)) == GENERAL_REGS))
+             && GET_CODE (op1) == REG
+             && REGNO (op1) < FIRST_PSEUDO_REGISTER
+             && REGNO_REG_CLASS (REGNO (op1)) != GENERAL_REGS))
         tem = op0, op0 = op1, op1 = tem;

       gen_reload (out, op0, opnum, type);

What I'm not quite sure about is if and how we can enter here with float 
operands.


>   > I submitted a patch to replace the gen_add2_insn() with a general 
> gen_rtx
>   > expression to avoid the abort, but Jeff doesn't like it :-(.
>I don't like it because it's the wrong solution.  You're fixing the symptom
>(gen_add2_insn aborting), not the problem (we never should have called
>gen_add2_insn with bogus operands).

Jeff, I only wanted you to elaborate a bit on _why_ it's wrong. Why is a 
general gen_rtx_SET ok for a 3-operand add, but not for a 2-operand add? 
Especially as the code immediately following is the same for both. I've 
heard no argument on that so far. Changing the swap behaviour was in fact 
my first approach, but looking at the rest of gen_reload the gen_rtx_SET 
looked a lot more natural and general.

>   > So my questions are:
>   > - is gen_reload() supposed to handle such an add with 3 regs/2 
> regclasses?
>Yes.  If it does not, then that would be the real bug IMHO.  If you don't
>have a 3 operand add, then you have to generate a move; add sequence.   It
>may be the case that we do not perform the necessary operand shuffling when
>we create the move;add sequence.

Ok.

Franz.

Re: Problem in gen_reload(), seeking an explanation

[Franz Sirl]

Am Mon, 21 Jun 1999 schrieb Joern Rennecke:
>> Hmm, wouldn't addsi3 abort() the same way as addsi2 does now? addsi2
>> currently aborts before we can try the move/add sequence a 2nd time with the
>> operands swapped.
>
>I'm not proposing to use addsi3 itself, but just using its operand predicates.
>Or more exactly, you index add_optab->handlers with the mode of the add,
>and if the insn code you get is not CODE_FOR_nothing, you call
>(*insn_operand_predicate[icode][1])

Hmm, something like this?

--- reload1.c   1999/05/08 01:34:55     1.145
+++ reload1.c   1999/06/21 20:30:19
@@ -7819,9 +7819,13 @@ gen_reload (out, in, opnum, type)
         DEFINE_PEEPHOLE should be specified that recognizes the sequence
         we emit below.  */

+      code = (int) add_optab->handlers[(int) GET_MODE (out)].insn_code;
+
       if (CONSTANT_P (op1) || GET_CODE (op1) == MEM || GET_CODE (op1) == SUBREG
          || (GET_CODE (op1) == REG
-             && REGNO (op1) >= FIRST_PSEUDO_REGISTER))
+             && REGNO (op1) >= FIRST_PSEUDO_REGISTER)
+         || (code != CODE_FOR_nothing
+             && ! (*insn_operand_predicate[code][2]) (op1, insn_operand_mode[code][2])))
        tem = op0, op0 = op1, op1 = tem;

       gen_reload (out, op0, opnum, type);

Actually I believe this solution is way better than the regclass approach. Do I
have to guard this with a (GET_CODE (op1) == REG && REGNO (op1) <
FIRST_PSEUDO_REGISTER)?

Franz.

Re: Problem in gen_reload(), seeking an explanation

[Jeffrey A Law]

  In message < 99062122402601.00461@ns1102.munich.netsurf.de >you write:
  > Am Mon, 21 Jun 1999 schrieb Joern Rennecke:
  > >> Hmm, wouldn't addsi3 abort() the same way as addsi2 does now? addsi2
  > >> currently aborts before we can try the move/add sequence a 2nd time with
  >  the
  > >> operands swapped.
  > >
  > >I'm not proposing to use addsi3 itself, but just using its operand predica
  > tes.
  > >Or more exactly, you index add_optab->handlers with the mode of the add,
  > >and if the insn code you get is not CODE_FOR_nothing, you call
  > >(*insn_operand_predicate[icode][1])
  > 
  > Hmm, something like this?
[ ... ]
Much better :-)  No mucking around with regclasses and generally cleaner.

Basically the updated patch says "if op1 is not a suitable operand for an
add instruction, then try to reload it into a suitable add operand".  Which
is what we really want to happen.

ie, we can reload either OP0 or OP1 into OUT.  We want to reload the one that
is not a valid add operand.  If neither is a valid add operand, then, well
we've got a real problem :-)  I do not think we need to worry about hard vs
pseudo regs here for this case

Did the revised patch fix your problem?

jeff

Re: Problem in gen_reload(), seeking an explanation

[Joern Rennecke]

> Hmm, wouldn't addsi3 abort() the same way as addsi2 does now? addsi2
> currently aborts before we can try the move/add sequence a 2nd time with the
> operands swapped.

I'm not proposing to use addsi3 itself, but just using its operand predicates.
Or more exactly, you index add_optab->handlers with the mode of the add,
and if the insn code you get is not CODE_FOR_nothing, you call
(*insn_operand_predicate[icode][1])

Re: Problem in gen_reload(), seeking an explanation

[Jeffrey A Law]

  In message < 4.2.0.56.19990616181739.039cabd0@mail.lauterbach.com >you write:
  > Hi,
  > 
  > on PPC gcc-2.95pre I have a testcase that aborts in gen_add2_insn() called 
  > by gen_reload(). The problem is a insn with 3 different real registers like
  >  
  > this
  > 
  > REG_CLASS(GENERAL_REGS) = REG_CLASS(CTR_REGS) + REG_CLASS(GENERAL_REGS)
  > 
  > There's no instruction on PPC that can handle these regclasses a single 
  > instruction add, so gen_reload() tries to use a move/add sequence. And here
  >  
  > starts the problem, cause it first tries
  > 
  > move GENERAL_REGS,GENERAL_REGS
  > add GENERAL_REGS,CTR_REGS
  > 
  > with the add generated by gen_add2_insn(). Unfortunately gen_add2_insn() 
  > aborts here, cause there is no "add GENERAL_REGS,CTR_REGS" on PPC.
  > But there is code in gen_reload() lateron that would try the following:
What I have maintained all along is that this should have been handled before
we called gen_add2_insn.  

If you look earlier in gen_reload we have support for rearranging the operands
of an add instruction to avoid this kind of problem.  You need to determine
why that code is not shuffling the operands so that the add looks like
GPRx = GPRx + CTR


  > I submitted a patch to replace the gen_add2_insn() with a general gen_rtx 
  > expression to avoid the abort, but Jeff doesn't like it :-(.
I don't like it because it's the wrong solution.  You're fixing the symptom
(gen_add2_insn aborting), not the problem (we never should have called 
gen_add2_insn with bogus operands).



  > So my questions are:
  > - is gen_reload() supposed to handle such an add with 3 regs/2 regclasses?
Yes.  If it does not, then that would be the real bug IMHO.  If you don't
have a 3 operand add, then you have to generate a move; add sequence.   It
may be the case that we do not perform the necessary operand shuffling when
we create the move;add sequence. 

jeff

Re: Problem in gen_reload(), seeking an explanation

[Franz Sirl]

Am Mon, 21 Jun 1999 schrieb Joern Rennecke:
>> OK, understood. Do I have to care about a FLOAT_REGS = FLOAT_REGS + 
>> GENERAL_REGS case here? Just to make sure I'm not breaking anything.
>
>reload shouldn't crate floating point adds, if that's what you mean.
>
>> >but that will still not handle the case properly when an add can
>> >operate on some register class that is not a subset of GENERAL_REGS.
>> >I don't know if we have any target where this is an issue, though.
>> 
>> Uhm, such an argument would probably point to a solution with 
>> gen_rtx_SET(gen_rtx_PLUS()) instead of gen_add2_insn for insn validation 
>> again? Otherwise you would have to put a lot of checking into the swap 
>> decision and probably need a new backend macro.
>
>We could also do the swap if the first operand of the add doesn't satisfies
>the predicate for operand 1 of the addsi3 pattern / expander, but the
>second operand of the add does.  As long as the predicates are defined
>sensible, this will help.

Hmm, wouldn't addsi3 abort() the same way as addsi2 does now? addsi2
currently aborts before we can try the move/add sequence a 2nd time with the
operands swapped.

Franz.

Re: Problem in gen_reload(), seeking an explanation

[Joern Rennecke]

> Hmm, something like this?
> 
> --- reload1.c   1999/05/08 01:34:55     1.145
> +++ reload1.c   1999/06/21 20:30:19
> @@ -7819,9 +7819,13 @@ gen_reload (out, in, opnum, type)
>          DEFINE_PEEPHOLE should be specified that recognizes the sequence
>          we emit below.  */
> 
> +      code = (int) add_optab->handlers[(int) GET_MODE (out)].insn_code;
> +
>        if (CONSTANT_P (op1) || GET_CODE (op1) == MEM || GET_CODE (op1) == SUBREG
>           || (GET_CODE (op1) == REG
> -             && REGNO (op1) >= FIRST_PSEUDO_REGISTER))
> +             && REGNO (op1) >= FIRST_PSEUDO_REGISTER)
> +         || (code != CODE_FOR_nothing
> +             && ! (*insn_operand_predicate[code][2]) (op1, insn_operand_mode[code][2])))
>         tem = op0, op0 = op1, op1 = tem;
> 
>        gen_reload (out, op0, opnum, type);

Yes.  The comment above the patch would have to be updated, of course.

> Actually I believe this solution is way better than the regclass approach. Do I
> have to guard this with a (GET_CODE (op1) == REG && REGNO (op1) <
> FIRST_PSEUDO_REGISTER)?

No, that shouldn't be necessary.  If the predicate returns false, the
add can't be recognized.

Re: Problem in gen_reload(), seeking an explanation

[Jeffrey A Law]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 326 bytes --]

  In message <4.2.0.56.19990625144600.037907f0@mail.lauterbach.com>you write:
[ ... gen_reload stuff ... ]
  > Yes, the problem is fixed with this patch :-). No testsuite regressions. 
Excellent.

  > This was a difficult one. What should I write in the comment Jörn 
requested?
I'll go ahead and update the comment.

jeff

Re: Problem in gen_reload(), seeking an explanation

[Jeffrey A Law]

  In message <4.2.0.56.19990622094640.03e97b10@mail.lauterbach.com>you write:
[ ... ]

  > You are kidding, aren't you? That's exactly what my first patch did.

  > >       [ ... check that the operands match the constraints... ]
  > >       if constraints fail, call delete_all_insns_since (last), swap
  > >       op0 and op1 and try again.  If it still fails delete the insns and
  > >       go on to the next method of generating the reload.
  > 
  > Still the same way as I suggested in my first patch.
  > 
  > Now, why didn't you accept my first patch?
It just means that after further examination of the problem that I changed my
mind about the best way to solve the problem.  It happens sometimes.

Regardless, you wrote a better patch using one of Joern's suggestions, so this
is a moot point.

jeff

Re: Problem in gen_reload(), seeking an explanation

[Jeffrey A Law]

  In message <4.2.0.56.19990622094640.03e97b10@mail.lauterbach.com>you write:
[ ... ]

  > You are kidding, aren't you? That's exactly what my first patch did.

  > >       [ ... check that the operands match the constraints... ]
  > >       if constraints fail, call delete_all_insns_since (last), swap
  > >       op0 and op1 and try again.  If it still fails delete the insns and
  > >       go on to the next method of generating the reload.
  > 
  > Still the same way as I suggested in my first patch.
  > 
  > Now, why didn't you accept my first patch?
It just means that after further examination of the problem that I changed my
mind about the best way to solve the problem.  It happens sometimes.

Regardless, you wrote a better patch using one of Joern's suggestions, so this
is a moot point.

jeff

Re: Problem in gen_reload(), seeking an explanation

[Jeffrey A Law]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 326 bytes --]

  In message <4.2.0.56.19990625144600.037907f0@mail.lauterbach.com>you write:
[ ... gen_reload stuff ... ]
  > Yes, the problem is fixed with this patch :-). No testsuite regressions. 
Excellent.

  > This was a difficult one. What should I write in the comment Jörn 
requested?
I'll go ahead and update the comment.

jeff