[rz@linux-m68k.org: Re: m68k, extendqidi2 problem]

[rz@linux-m68k.org: Re: m68k, extendqidi2 problem]

[Richard Zidlicky]

[-- Attachment #1: Type: text/plain, Size: 312 bytes --]

Hi,

sorry, I messed up something in .muttrc so the appended patch
went out only privately to Richard Henderson. 
Richard Henderson gave it already the blessing so please check
it in.

Richard

2004-03-16 Richard Zidlicky <rz@linux-m68k.org>
	* config/m68k/m68k.md: Avoid generating illegal code in extendqidi2


[-- Attachment #2: Type: message/rfc822, Size: 1715 bytes --]

From: Richard Zidlicky <rz@linux-m68k.org>
To: Richard Henderson <rth@redhat.com>
Subject: Re: m68k, extendqidi2 problem
Date: Mon, 8 Mar 2004 12:38:02 +0100
Message-ID: <20040308113802.GA4125@linux-m68k.org>

On Sat, Mar 06, 2004 at 11:27:07AM -0800, Richard Henderson wrote:
> On Sat, Mar 06, 2004 at 12:21:26AM +0100, Richard Zidlicky wrote:
> > It seems we can keep the constraint as is and make a move.w when
> > %1 is ADDRESS_REG_P...
> 
> That seems ok.

here is the patch

--- gcc-3.4-20040218/gcc/config/m68k/m68k.md.rz-extbdi	2004-03-07 23:08:04.000000000 +0100
+++ gcc-3.4-20040218/gcc/config/m68k/m68k.md	2004-03-07 23:32:21.000000000 +0100
@@ -1455,10 +1455,20 @@
 {
   CC_STATUS_INIT;
   operands[2] = gen_rtx_REG (SImode, REGNO (operands[0]) + 1);
-  if (TARGET_68020 || TARGET_COLDFIRE)
-    return "move%.b %1,%2\;extb%.l %2\;smi %0\;extb%.l %0";
+  if (ADDRESS_REG_P(operands[1]))
+    {
+      if (TARGET_68020 || TARGET_COLDFIRE)
+        return "move%.w %1,%2\;extb%.l %2\;smi %0\;extb%.l %0";
+      else
+        return "move%.w %1,%2\;ext%.w %0\;ext%.l %2\;move%.l %2,%0\;smi %0";
+    }
   else
-    return "move%.b %1,%2\;ext%.w %0\;ext%.l %2\;move%.l %2,%0\;smi %0";
+    {
+      if (TARGET_68020 || TARGET_COLDFIRE)
+        return "move%.b %1,%2\;extb%.l %2\;smi %0\;extb%.l %0";
+      else
+        return "move%.b %1,%2\;ext%.w %0\;ext%.l %2\;move%.l %2,%0\;smi %0";
+    }
 })
 
 (define_insn "extendhidi2"

Re: [rz@linux-m68k.org: Re: m68k, extendqidi2 problem]

[Gunther Nikl]

On Tue, Mar 16, 2004 at 10:56:49PM +0100, Richard Zidlicky wrote:
> --- gcc-3.4-20040218/gcc/config/m68k/m68k.md.rz-extbdi	2004-03-07 23:08:04.000000000 +0100
> +++ gcc-3.4-20040218/gcc/config/m68k/m68k.md	2004-03-07 23:32:21.000000000 +0100
> @@ -1455,10 +1455,20 @@
>  {
>    CC_STATUS_INIT;
>    operands[2] = gen_rtx_REG (SImode, REGNO (operands[0]) + 1);
> -  if (TARGET_68020 || TARGET_COLDFIRE)
> -    return "move%.b %1,%2\;extb%.l %2\;smi %0\;extb%.l %0";
> +  if (ADDRESS_REG_P(operands[1]))
> +    {
> +      if (TARGET_68020 || TARGET_COLDFIRE)
> +        return "move%.w %1,%2\;extb%.l %2\;smi %0\;extb%.l %0";
> +      else
> +        return "move%.w %1,%2\;ext%.w %0\;ext%.l %2\;move%.l %2,%0\;smi %0";

  Why "ext%.w %0"? Shouldn't it be "ext%.w %2"? Then I don't understand
  the "move%.l %2,%0 in the else case and the extbl after smi in the if
  case.

> +    }
>    else
> -    return "move%.b %1,%2\;ext%.w %0\;ext%.l %2\;move%.l %2,%0\;smi %0";
> +    {
> +      if (TARGET_68020 || TARGET_COLDFIRE)
> +        return "move%.b %1,%2\;extb%.l %2\;smi %0\;extb%.l %0";
> +      else
> +        return "move%.b %1,%2\;ext%.w %0\;ext%.l %2\;move%.l %2,%0\;smi %0";

  Same here.

  Gunther

Re: [rz@linux-m68k.org: Re: m68k, extendqidi2 problem]

[Jim Wilson]

On Wed, 2004-03-17 at 04:17, Gunther Nikl wrote:
>   Why "ext%.w %0"? Shouldn't it be "ext%.w %2"? Then I don't understand
>   the "move%.l %2,%0 in the else case and the extbl after smi in the if
>   case.

If you actually look at the extendqidi2 pattern, it should make more
sense.  Otherwise, you are lacking context about what op0 and op2 are. 
Op2 is the low word of the result, which contains the original byte
sign-extended to a work.  op0 is the upper word, which contains only
sign-extension bits.  The sign extension bits can be generated by smi. 
We can either use smi and then extend it (if case), or we can move the
low word to the high word, and then replace the original byte in place
with sign-extend bits (else case) which gives an entire word of
sign-extend bits.  This assumes smi does a strict_low_part set, which is
a reasonable assumption for a m68k target.

But yes, I believe you are right about the ext%.w %0 in the else case.
It should be ext%.w %2.  This is a bug in the original code before
Richard Zidicky's patch, which then got replicated in a second place by
his patch.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: [rz@linux-m68k.org: Re: m68k, extendqidi2 problem]

[Richard Zidlicky]

On Wed, Mar 17, 2004 at 01:17:32PM +0100, Gunther Nikl wrote:
> > +      if (TARGET_68020 || TARGET_COLDFIRE)
> > +        return "move%.w %1,%2\;extb%.l %2\;smi %0\;extb%.l %0";
> > +      else
> > +        return "move%.w %1,%2\;ext%.w %0\;ext%.l %2\;move%.l %2,%0\;smi %0";
> 
>   Why "ext%.w %0"? Shouldn't it be "ext%.w %2"? Then I don't understand
>   the "move%.l %2,%0 in the else case and the extbl after smi in the if
>   case.

extb.l takes a byte and extends to long. ext.w extends byte to 16 bit.
It would work but needs an extra insn

> > +   return "move%.w %1,%2\;ext%.w %0\;ext%.l %2\;move%.l %2,%0\;smi %0";
                                ^^^^^^^^^

Thinking more about your question.. does that look like yet another bug
in the same pattern ?! 

So lets assume

> > +   return "move%.w %1,%2\;ext%.w %2\;ext%.l %2\;move%.l %2,%0\;smi %0";

..  makes more sense to me. The "move.l %2,%0" will set or clear upper 24
bits, smi the lower 8 bits of the high word register. Pure curiosity, why
isn't the same trick used in the 68020 branch?

Has that pattern ever been used before? Any more bugs in it?
I have now tried to convert it to "%R" notation.. 

(define_insn "extendqidi2"
  [(set (match_operand:DI 0 "nonimmediate_operand" "=d")
        (sign_extend:DI (match_operand:QI 1 "general_src_operand" "rmS")))]
  ""
{
  CC_STATUS_INIT;
  if (ADDRESS_REG_P(operands[1]))
    {
      if (TARGET_68020 || TARGET_COLDFIRE)
        return "move%.w %1,%R0\;extb%.l %R0\;smi %0\;extb%.l %0";
      else
        return "move%.w %1,%R0\;ext%.w %R0\;ext%.l %R0\;move%.l %R0,%0\;smi %0";
    }
  else
    {
      if (TARGET_68020 || TARGET_COLDFIRE)
        return "move%.b %1,%R0\;extb%.l %R0\;smi %0\;extb%.l %0";
      else
        return "move%.b %1,%R0\;ext%.w %R0\;ext%.l %R0\;move%.l %R0,%0\;smi %0";
    }
})



Richard

Re: [rz@linux-m68k.org: Re: m68k, extendqidi2 problem]

[Jim Wilson]

On Wed, 2004-03-17 at 14:11, Richard Zidlicky wrote:
> ..  makes more sense to me. The "move.l %2,%0" will set or clear upper 24
> bits, smi the lower 8 bits of the high word register. Pure curiosity, why
> isn't the same trick used in the 68020 branch?

Looking at gcc-2.7, we see that it was first
  if (TARGET_68020)
    return \"move%.b %1,%2\;extb%.l %2\;smi %0\;extb%.l %0\";
  else
    return \"move%.b %1,%2\;ext%.w %0\;ext%.l %2\;smi %0\;ext%.w
%0\;ext%.l %0\\";
and then between between 2.7 and 2.8 the non-68020 case was optimized to
delete one instruction by using this trick.  Not clear when exactly this
happened, just by looking at the ChangeLogs.

Perhaps the 68020 case was left alone to avoid the risk of breaking it. 
Or maybe it was left alone because smi/ext is a lot easier to understand
than movl/smi.

Your revised pattern looks right to me.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: [rz@linux-m68k.org: Re: m68k, extendqidi2 problem]

[Gunther Nikl]

On Wed, Mar 17, 2004 at 11:11:31PM +0100, Richard Zidlicky wrote:
> On Wed, Mar 17, 2004 at 01:17:32PM +0100, Gunther Nikl wrote:
> > > +      if (TARGET_68020 || TARGET_COLDFIRE)
> > > +        return "move%.w %1,%2\;extb%.l %2\;smi %0\;extb%.l %0";
> > > +      else
> > > +        return "move%.w %1,%2\;ext%.w %0\;ext%.l %2\;move%.l %2,%0\;smi %0";
> > 
> >   Why "ext%.w %0"? Shouldn't it be "ext%.w %2"? Then I don't understand
> >   the "move%.l %2,%0 in the else case and the extbl after smi in the if
> >   case.
> 
> extb.l takes a byte and extends to long. ext.w extends byte to 16 bit.
> It would work but needs an extra insn

  I know. I was talking about movel+smi and smi+extbl.

> > > +   return "move%.w %1,%2\;ext%.w %0\;ext%.l %2\;move%.l %2,%0\;smi %0";
>                                 ^^^^^^^^^
> Thinking more about your question.. does that look like yet another bug
> in the same pattern ?! 
> 
> So lets assume
> 
> > > +   return "move%.w %1,%2\;ext%.w %2\;ext%.l %2\;move%.l %2,%0\;smi %0";
> 
> ..  makes more sense to me. The "move.l %2,%0" will set or clear upper 24
> bits, smi the lower 8 bits of the high word register. Pure curiosity, why
> isn't the same trick used in the 68020 branch?

  I would use it also in the 68020 case. Then the only difference between
  the two cases is extw+extl vs. extbl. I guess movel is faster than extbl
  but maybe smi+extbl is better for pipelining. In that case your new
  version would be fine.

> Has that pattern ever been used before? Any more bugs in it?
> I have now tried to convert it to "%R" notation.. 
> 
> (define_insn "extendqidi2"
>   [(set (match_operand:DI 0 "nonimmediate_operand" "=d")
>         (sign_extend:DI (match_operand:QI 1 "general_src_operand" "rmS")))]
>   ""
> {
>   CC_STATUS_INIT;
>   if (ADDRESS_REG_P(operands[1]))
>     {
>       if (TARGET_68020 || TARGET_COLDFIRE)
>         return "move%.w %1,%R0\;extb%.l %R0\;smi %0\;extb%.l %0";
>       else
>         return "move%.w %1,%R0\;ext%.w %R0\;ext%.l %R0\;move%.l %R0,%0\;smi %0";
>     }
>   else
>     {
>       if (TARGET_68020 || TARGET_COLDFIRE)
>         return "move%.b %1,%R0\;extb%.l %R0\;smi %0\;extb%.l %0";
>       else
>         return "move%.b %1,%R0\;ext%.w %R0\;ext%.l %R0\;move%.l %R0,%0\;smi %0";
>     }
> })

  Gunther

Re: [rz@linux-m68k.org: Re: m68k, extendqidi2 problem]

[Andreas Schwab]

Jim Wilson <wilson@specifixinc.com> writes:

> Perhaps the 68020 case was left alone to avoid the risk of breaking it. 
> Or maybe it was left alone because smi/ext is a lot easier to understand
> than movl/smi.

"move%.b %1,%R0;extb%.l %R0;move%.l %R0,%0;smi %0" should be at least as
fast as "move%.b %1,%R0;extb%.l %R0;smi %0;extb%.l %0" whereever it is
applicable, but the former is faster at least on 68020 and 68030.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."