[PATCH] Don't check for optab for 16bit bswap

[PATCH] Don't check for optab for 16bit bswap

[Thomas Preud'homme]

Since 16bit byteswap can be done via an 8 bit rotation (and it is the canonical form),
the check for an optab only serves to prevent the bswap optimization for
targets that don't have a 16bit byteswap (but do have a rotation instruction). See
PR63259 (comments 6 and onwards) for more details.

ChangeLog entry is as follows:

2014-12-24  Thomas Preud'homme  thomas.preudhomme@arm.com

PR tree-optimization/63259
* tree-ssa-math-opts.c (pass_optimize_bswap::execute): Stop checking
if optab exists for 16bit byteswap.


diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
index 1ed2838..c175e88 100644
--- a/gcc/tree-ssa-math-opts.c
+++ b/gcc/tree-ssa-math-opts.c
@@ -2350,15 +2350,13 @@ unsigned int
 pass_optimize_bswap::execute (function *fun)
 {
   basic_block bb;
-  bool bswap16_p, bswap32_p, bswap64_p;
+  bool bswap32_p, bswap64_p;
   bool changed = false;
-  tree bswap16_type = NULL_TREE, bswap32_type = NULL_TREE, bswap64_type = NULL_TREE;
+  tree bswap32_type = NULL_TREE, bswap64_type = NULL_TREE;
 
   if (BITS_PER_UNIT != 8)
     return 0;
 
-  bswap16_p = (builtin_decl_explicit_p (BUILT_IN_BSWAP16)
-	       && optab_handler (bswap_optab, HImode) != CODE_FOR_nothing);
   bswap32_p = (builtin_decl_explicit_p (BUILT_IN_BSWAP32)
 	       && optab_handler (bswap_optab, SImode) != CODE_FOR_nothing);
   bswap64_p = (builtin_decl_explicit_p (BUILT_IN_BSWAP64)
@@ -2367,12 +2365,6 @@ pass_optimize_bswap::execute (function *fun)
 
   /* Determine the argument type of the builtins.  The code later on
      assumes that the return and argument type are the same.  */
-  if (bswap16_p)
-    {
-      tree fndecl = builtin_decl_explicit (BUILT_IN_BSWAP16);
-      bswap16_type = TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (fndecl)));
-    }
-
   if (bswap32_p)
     {
       tree fndecl = builtin_decl_explicit (BUILT_IN_BSWAP32);
@@ -2443,11 +2435,6 @@ pass_optimize_bswap::execute (function *fun)
 	      if (code == LROTATE_EXPR || code == RROTATE_EXPR)
 		continue;
 	      load_type = uint16_type_node;
-	      if (bswap16_p)
-		{
-		  fndecl = builtin_decl_explicit (BUILT_IN_BSWAP16);
-		  bswap_type = bswap16_type;
-		}
 	      break;
 	    case 32:
 	      load_type = uint32_type_node;
@@ -2469,7 +2456,7 @@ pass_optimize_bswap::execute (function *fun)
 	      continue;
 	    }
 
-	  if (bswap && !fndecl)
+	  if (bswap && !fndecl && n.range != 16)
 	    continue;
 
 	  if (bswap_replace (cur_stmt, src_stmt, fndecl, bswap_type, load_type,

Bootstrapped on x86_64-linux-gnu and aarch64-none-linux-gnu without any
testsuite regression

Is this ok for trunk?

Best regards,

Thomas

Re: [PATCH] Don't check for optab for 16bit bswap

[Richard Biener]

On December 29, 2014 3:04:36 PM CET, Thomas Preud'homme <thomas.preudhomme@arm.com> wrote:
>Since 16bit byteswap can be done via an 8 bit rotation (and it is the
>canonical form),
>the check for an optab only serves to prevent the bswap optimization
>for
>targets that don't have a 16bit byteswap (but do have a rotation
>instruction). See
>PR63259 (comments 6 and onwards) for more details.

OK, but what about targets without a rotation optab?  Is the fallback expansion reasonable in all cases?

Richard.

>ChangeLog entry is as follows:
>
>2014-12-24  Thomas Preud'homme  thomas.preudhomme@arm.com
>
>PR tree-optimization/63259
>* tree-ssa-math-opts.c (pass_optimize_bswap::execute): Stop checking
>if optab exists for 16bit byteswap.
>
>
>diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
>index 1ed2838..c175e88 100644
>--- a/gcc/tree-ssa-math-opts.c
>+++ b/gcc/tree-ssa-math-opts.c
>@@ -2350,15 +2350,13 @@ unsigned int
> pass_optimize_bswap::execute (function *fun)
> {
>   basic_block bb;
>-  bool bswap16_p, bswap32_p, bswap64_p;
>+  bool bswap32_p, bswap64_p;
>   bool changed = false;
>-  tree bswap16_type = NULL_TREE, bswap32_type = NULL_TREE,
>bswap64_type = NULL_TREE;
>+  tree bswap32_type = NULL_TREE, bswap64_type = NULL_TREE;
> 
>   if (BITS_PER_UNIT != 8)
>     return 0;
> 
>-  bswap16_p = (builtin_decl_explicit_p (BUILT_IN_BSWAP16)
>-	       && optab_handler (bswap_optab, HImode) != CODE_FOR_nothing);
>   bswap32_p = (builtin_decl_explicit_p (BUILT_IN_BSWAP32)
> 	       && optab_handler (bswap_optab, SImode) != CODE_FOR_nothing);
>   bswap64_p = (builtin_decl_explicit_p (BUILT_IN_BSWAP64)
>@@ -2367,12 +2365,6 @@ pass_optimize_bswap::execute (function *fun)
> 
>   /* Determine the argument type of the builtins.  The code later on
>      assumes that the return and argument type are the same.  */
>-  if (bswap16_p)
>-    {
>-      tree fndecl = builtin_decl_explicit (BUILT_IN_BSWAP16);
>-      bswap16_type = TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (fndecl)));
>-    }
>-
>   if (bswap32_p)
>     {
>       tree fndecl = builtin_decl_explicit (BUILT_IN_BSWAP32);
>@@ -2443,11 +2435,6 @@ pass_optimize_bswap::execute (function *fun)
> 	      if (code == LROTATE_EXPR || code == RROTATE_EXPR)
> 		continue;
> 	      load_type = uint16_type_node;
>-	      if (bswap16_p)
>-		{
>-		  fndecl = builtin_decl_explicit (BUILT_IN_BSWAP16);
>-		  bswap_type = bswap16_type;
>-		}
> 	      break;
> 	    case 32:
> 	      load_type = uint32_type_node;
>@@ -2469,7 +2456,7 @@ pass_optimize_bswap::execute (function *fun)
> 	      continue;
> 	    }
> 
>-	  if (bswap && !fndecl)
>+	  if (bswap && !fndecl && n.range != 16)
> 	    continue;
> 
>	  if (bswap_replace (cur_stmt, src_stmt, fndecl, bswap_type,
>load_type,
>
>Bootstrapped on x86_64-linux-gnu and aarch64-none-linux-gnu without any
>testsuite regression
>
>Is this ok for trunk?
>
>Best regards,
>
>Thomas

RE: [PATCH] Don't check for optab for 16bit bswap

[Thomas Preud'homme]

> From: Richard Biener [mailto:rguenther@suse.de]
> Sent: Monday, December 29, 2014 5:09 PM
> 
> OK, but what about targets without a rotation optab?  Is the fallback
> expansion reasonable in all cases?

To be honest I haven't checked. I thought being a treecode means it
can always be expanded, using a sequence of shift and bitwise or if
necessary. Isn't there some language that GCC support with rotate
operators?

Given your question I guess I was wrong assuming this. Is there a list
of gimple construct that are necessary supported? What about a list
of insn pattern that a backend must necessarily provide?

Best regards,

Thomas 

RE: [PATCH] Don't check for optab for 16bit bswap

[Oleg Endo]

On Mon, 2014-12-29 at 17:53 +0000, Thomas Preud'homme wrote:
> > From: Richard Biener [mailto:rguenther@suse.de]
> > Sent: Monday, December 29, 2014 5:09 PM
> > 
> > OK, but what about targets without a rotation optab?  Is the fallback
> > expansion reasonable in all cases?
> 
> To be honest I haven't checked. I thought being a treecode means it
> can always be expanded, using a sequence of shift and bitwise or if
> necessary. Isn't there some language that GCC support with rotate
> operators?
> 
> Given your question I guess I was wrong assuming this. Is there a list
> of gimple construct that are necessary supported? What about a list
> of insn pattern that a backend must necessarily provide?
> 

__builtin_bswap16 expansion uses the 'rotlhi3' pattern to do a 16 bit
bswap as a fallback when there's no 'bswaphi2' pattern in the backend
(like on SH at the moment.  I haven't added bswaphi2, as
__builtin_bswap16 has been working without it).

I've just tried disabling the 'rotlhi3' pattern and __builtin_bswap16
expands into shift + and + or (as expected).
Thus, I don't think the patch will make something worse (than it already
is) on some backends.  If the bswap detection bails out at the tree
level, the expanded ops will be shift + and + or -- as written in the
original code.  So probably, that will be the same as the fallback
expansion for __builtin_bswap16, and we're back to sqrt (1).

Cheers,
Oleg

RE: [PATCH] Don't check for optab for 16bit bswap

[Richard Biener]

On December 29, 2014 7:44:13 PM CET, Oleg Endo <oleg.endo@t-online.de> wrote:
>On Mon, 2014-12-29 at 17:53 +0000, Thomas Preud'homme wrote:
>> > From: Richard Biener [mailto:rguenther@suse.de]
>> > Sent: Monday, December 29, 2014 5:09 PM
>> > 
>> > OK, but what about targets without a rotation optab?  Is the
>fallback
>> > expansion reasonable in all cases?
>> 
>> To be honest I haven't checked. I thought being a treecode means it
>> can always be expanded, using a sequence of shift and bitwise or if
>> necessary. Isn't there some language that GCC support with rotate
>> operators?
>> 
>> Given your question I guess I was wrong assuming this. Is there a
>list
>> of gimple construct that are necessary supported? What about a list
>> of insn pattern that a backend must necessarily provide?
>> 
>
>__builtin_bswap16 expansion uses the 'rotlhi3' pattern to do a 16 bit
>bswap as a fallback when there's no 'bswaphi2' pattern in the backend
>(like on SH at the moment.  I haven't added bswaphi2, as
>__builtin_bswap16 has been working without it).
>
>I've just tried disabling the 'rotlhi3' pattern and __builtin_bswap16
>expands into shift + and + or (as expected).
>Thus, I don't think the patch will make something worse (than it
>already
>is) on some backends.  If the bswap detection bails out at the tree
>level, the expanded ops will be shift + and + or -- as written in the
>original code.  So probably, that will be the same as the fallback
>expansion for __builtin_bswap16, and we're back to sqrt (1).

OK, that is what I was asking - are there cases where using rot is worse (like forcing a libcall or so).

Richard.

>Cheers,
>Oleg

RE: [PATCH] Don't check for optab for 16bit bswap

[Oleg Endo]

On Tue, 2014-12-30 at 16:22 +0100, Richard Biener wrote:
> On December 29, 2014 7:44:13 PM CET, Oleg Endo <oleg.endo@t-online.de> wrote:
> >On Mon, 2014-12-29 at 17:53 +0000, Thomas Preud'homme wrote:
> >> > From: Richard Biener [mailto:rguenther@suse.de]
> >> > Sent: Monday, December 29, 2014 5:09 PM
> >> > 
> >> > OK, but what about targets without a rotation optab?  Is the
> >fallback
> >> > expansion reasonable in all cases?
> >> 
> >> To be honest I haven't checked. I thought being a treecode means it
> >> can always be expanded, using a sequence of shift and bitwise or if
> >> necessary. Isn't there some language that GCC support with rotate
> >> operators?
> >> 
> >> Given your question I guess I was wrong assuming this. Is there a
> >list
> >> of gimple construct that are necessary supported? What about a list
> >> of insn pattern that a backend must necessarily provide?
> >> 
> >
> >__builtin_bswap16 expansion uses the 'rotlhi3' pattern to do a 16 bit
> >bswap as a fallback when there's no 'bswaphi2' pattern in the backend
> >(like on SH at the moment.  I haven't added bswaphi2, as
> >__builtin_bswap16 has been working without it).
> >
> >I've just tried disabling the 'rotlhi3' pattern and __builtin_bswap16
> >expands into shift + and + or (as expected).
> >Thus, I don't think the patch will make something worse (than it
> >already

.L42:

> >is) on some backends.  If the bswap detection bails out at the tree
> >level, the expanded ops will be shift + and + or -- as written in the
> >original code.  So probably, that will be the same as the fallback
> >expansion for __builtin_bswap16, and we're back to sqrt (1).
> 
> OK, that is what I was asking - are there cases where using rot is worse (like forcing a libcall or so).

See also comment in expmed.c (expand_shift_1):
 It is theoretically possible that the target machine might
 not be able to perform either shift and hence we would
 be making two libcalls rather than just the one for the
 shift (similarly if IOR could not be done).  We will allow
 this extremely unlikely lossage to avoid complicating the
 code below.  */

then goto .L42  ;)

Cheers,
Oleg

RE: [PATCH] Don't check for optab for 16bit bswap

[Thomas Preud'homme]

> From: Oleg Endo [mailto:oleg.endo@t-online.de]
> Sent: Tuesday, December 30, 2014 4:25 PM
> 
> I've just tried disabling the 'rotlhi3' pattern and __builtin_bswap16
> expands into shift + and + or (as expected).
> Thus, I don't think the patch will make something worse (than it
> already
> 
> .L42:
> 
> is) on some backends.  If the bswap detection bails out at the tree
> level, the expanded ops will be shift + and + or -- as written in the
> original code.  So probably, that will be the same as the fallback
> expansion for __builtin_bswap16, and we're back to sqrt (1).
> 
> > OK, that is what I was asking - are there cases where using rot is worse
> (like forcing a libcall or so).
> 
> See also comment in expmed.c (expand_shift_1):
>  It is theoretically possible that the target machine might
>  not be able to perform either shift and hence we would
>  be making two libcalls rather than just the one for the
>  shift (similarly if IOR could not be done).  We will allow
>  this extremely unlikely lossage to avoid complicating the
>  code below.  */
> 
> then goto .L42  ;)

Yes and if the fallback expansion for a rot is actually worse than the
original set of stmt it means the fallback expansion should be improved.

Now I realize that said like this it sounds more like stage1 material.

Would it be fine for next stage1?

Best regards,

Thomas 

RE: [PATCH] Don't check for optab for 16bit bswap

[Oleg Endo]

On Mon, 2015-01-05 at 14:54 +0000, Thomas Preud'homme wrote:
> > From: Oleg Endo [mailto:oleg.endo@t-online.de]
> > Sent: Tuesday, December 30, 2014 4:25 PM
> > 
> > I've just tried disabling the 'rotlhi3' pattern and __builtin_bswap16
> > expands into shift + and + or (as expected).
> > Thus, I don't think the patch will make something worse (than it
> > already
> > 
> > .L42:
> > 
> > is) on some backends.  If the bswap detection bails out at the tree
> > level, the expanded ops will be shift + and + or -- as written in the
> > original code.  So probably, that will be the same as the fallback
> > expansion for __builtin_bswap16, and we're back to sqrt (1).
> > 
> > > OK, that is what I was asking - are there cases where using rot is worse
> > (like forcing a libcall or so).
> > 
> > See also comment in expmed.c (expand_shift_1):
> >  It is theoretically possible that the target machine might
> >  not be able to perform either shift and hence we would
> >  be making two libcalls rather than just the one for the
> >  shift (similarly if IOR could not be done).  We will allow
> >  this extremely unlikely lossage to avoid complicating the
> >  code below.  */
> > 
> > then goto .L42  ;)
> 
> Yes and if the fallback expansion for a rot is actually worse than the
> original set of stmt it means the fallback expansion should be improved.

I might be missing something, but how can the original shift/and/or
statements (which eventually are translated to shift/and/or insns) be
any better than the shift/and/or insns that are expanded by the rot
fallback?  In both cases the insns have to go through the backend
expanders, and if those are absent/unsupported libcalls will be
expanded.

> Now I realize that said like this it sounds more like stage1 material.
> 
> Would it be fine for next stage1?

Or maybe just leave everything as it is and I add a bswaphi expander in
the SH backend ;)

Cheers,
Oleg

RE: [PATCH] Don't check for optab for 16bit bswap

[Richard Biener]

On January 5, 2015 3:54:40 PM CET, Thomas Preud'homme <thomas.preudhomme@arm.com> wrote:
>> From: Oleg Endo [mailto:oleg.endo@t-online.de]
>> Sent: Tuesday, December 30, 2014 4:25 PM
>> 
>> I've just tried disabling the 'rotlhi3' pattern and __builtin_bswap16
>> expands into shift + and + or (as expected).
>> Thus, I don't think the patch will make something worse (than it
>> already
>> 
>> .L42:
>> 
>> is) on some backends.  If the bswap detection bails out at the tree
>> level, the expanded ops will be shift + and + or -- as written in the
>> original code.  So probably, that will be the same as the fallback
>> expansion for __builtin_bswap16, and we're back to sqrt (1).
>> 
>> > OK, that is what I was asking - are there cases where using rot is
>worse
>> (like forcing a libcall or so).
>> 
>> See also comment in expmed.c (expand_shift_1):
>>  It is theoretically possible that the target machine might
>>  not be able to perform either shift and hence we would
>>  be making two libcalls rather than just the one for the
>>  shift (similarly if IOR could not be done).  We will allow
>>  this extremely unlikely lossage to avoid complicating the
>>  code below.  */
>> 
>> then goto .L42  ;)
>
>Yes and if the fallback expansion for a rot is actually worse than the
>original set of stmt it means the fallback expansion should be
>improved.
>
>Now I realize that said like this it sounds more like stage1 material.
>
>Would it be fine for next stage1?

No, I think the patch is fine as-is now.

Thanks,
Richard.

>Best regards,
>
>Thomas 

Re: [PATCH] Don't check for optab for 16bit bswap

[H.J. Lu]

On Mon, Jan 5, 2015 at 1:09 PM, Richard Biener <rguenther@suse.de> wrote:
> On January 5, 2015 3:54:40 PM CET, Thomas Preud'homme <thomas.preudhomme@arm.com> wrote:
>>> From: Oleg Endo [mailto:oleg.endo@t-online.de]
>>> Sent: Tuesday, December 30, 2014 4:25 PM
>>>
>>> I've just tried disabling the 'rotlhi3' pattern and __builtin_bswap16
>>> expands into shift + and + or (as expected).
>>> Thus, I don't think the patch will make something worse (than it
>>> already
>>>
>>> .L42:
>>>
>>> is) on some backends.  If the bswap detection bails out at the tree
>>> level, the expanded ops will be shift + and + or -- as written in the
>>> original code.  So probably, that will be the same as the fallback
>>> expansion for __builtin_bswap16, and we're back to sqrt (1).
>>>
>>> > OK, that is what I was asking - are there cases where using rot is
>>worse
>>> (like forcing a libcall or so).
>>>
>>> See also comment in expmed.c (expand_shift_1):
>>>  It is theoretically possible that the target machine might
>>>  not be able to perform either shift and hence we would
>>>  be making two libcalls rather than just the one for the
>>>  shift (similarly if IOR could not be done).  We will allow
>>>  this extremely unlikely lossage to avoid complicating the
>>>  code below.  */
>>>
>>> then goto .L42  ;)
>>
>>Yes and if the fallback expansion for a rot is actually worse than the
>>original set of stmt it means the fallback expansion should be
>>improved.
>>
>>Now I realize that said like this it sounds more like stage1 material.
>>
>>Would it be fine for next stage1?
>
> No, I think the patch is fine as-is now.

Does it cause:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64718

-- 
H.J.

Re: [PATCH] Don't check for optab for 16bit bswap

[Richard Henderson]

On 12/29/2014 06:04 AM, Thomas Preud'homme wrote:
> Since 16bit byteswap can be done via an 8 bit rotation (and it is the canonical form),
> the check for an optab only serves to prevent the bswap optimization for
> targets that don't have a 16bit byteswap (but do have a rotation instruction). See
> PR63259 (comments 6 and onwards) for more details.

I question the choice to have rotate be the canonical form.

Doesn't this make things more complicated for targets that
don't have rotate?  Or, equivalently, no 16-bit rotate?
That would seem to cover 99% of all 32-bit risc machines.


r~

RE: [PATCH] Don't check for optab for 16bit bswap

[Thomas Preud'homme]

> -----Original Message-----
> From: Richard Henderson [mailto:rth@redhat.com]
> Sent: Thursday, January 22, 2015 5:24 AM
> To: Thomas Preud'homme; gcc-patches@gcc.gnu.org; 'Richard Biener'
> Subject: Re: [PATCH] Don't check for optab for 16bit bswap
> 
> On 12/29/2014 06:04 AM, Thomas Preud'homme wrote:
> > Since 16bit byteswap can be done via an 8 bit rotation (and it is the
> canonical form),
> > the check for an optab only serves to prevent the bswap optimization
> for
> > targets that don't have a 16bit byteswap (but do have a rotation
> instruction). See
> > PR63259 (comments 6 and onwards) for more details.
> 
> I question the choice to have rotate be the canonical form.
> 
> Doesn't this make things more complicated for targets that
> don't have rotate?  Or, equivalently, no 16-bit rotate?
> That would seem to cover 99% of all 32-bit risc machines.

Here is the difference between the two (see [1]):

/* Rotation of 16bit values by 8 bits is effectively equivalent to a bswaphi.
Note that this is not the case for bigger values. For instance a rotation
of 0x01020304 by 16 bits gives 0x03040102 which is different from
0x04030201 (bswapsi). */
if (rotate
     && CONST_INT_P (op1)
     && INTVAL (op1) == BITS_PER_UNIT
     && GET_MODE_SIZE (scalar_mode) == 2
     && optab_handler (bswap_optab, HImode) != CODE_FOR_nothing)
  return expand_unop (HImode, bswap_optab, shifted, NULL_RTX,
			unsignedp);

[1] https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/expmed.c?revision=219718&view=markup#l2208

It doesn't look much more complicated. There was also some change in
the bswap pass but some code was both added and removed. As to the
bug raised by H.J. Lu, I just wrote a patch and testing is underway.

Given this, I don't think it's a problem. That being said, I'm happy to
change the canonical representation if there is consensus for that.

Best regards,

Thomas

Re: [PATCH] Don't check for optab for 16bit bswap

[Richard Biener]

On January 21, 2015 10:23:56 PM CET, Richard Henderson <rth@redhat.com> wrote:
>On 12/29/2014 06:04 AM, Thomas Preud'homme wrote:
>> Since 16bit byteswap can be done via an 8 bit rotation (and it is the
>canonical form),
>> the check for an optab only serves to prevent the bswap optimization
>for
>> targets that don't have a 16bit byteswap (but do have a rotation
>instruction). See
>> PR63259 (comments 6 and onwards) for more details.
>
>I question the choice to have rotate be the canonical form.
>
>Doesn't this make things more complicated for targets that
>don't have rotate?  Or, equivalently, no 16-bit rotate?
>That would seem to cover 99% of all 32-bit risc machines.

I was asking for the generic expander to consider bswapHI.  Rotates are certainly more likely to get combined with sth else.

Richard.

>
>r~

Re: [PATCH] Don't check for optab for 16bit bswap

[Richard Henderson]

On 01/21/2015 11:52 PM, Richard Biener wrote:
> On January 21, 2015 10:23:56 PM CET, Richard Henderson <rth@redhat.com> wrote:
>> On 12/29/2014 06:04 AM, Thomas Preud'homme wrote:
>>> Since 16bit byteswap can be done via an 8 bit rotation (and it is the
>> canonical form),
>>> the check for an optab only serves to prevent the bswap optimization
>> for
>>> targets that don't have a 16bit byteswap (but do have a rotation
>> instruction). See
>>> PR63259 (comments 6 and onwards) for more details.
>>
>> I question the choice to have rotate be the canonical form.
>>
>> Doesn't this make things more complicated for targets that
>> don't have rotate?  Or, equivalently, no 16-bit rotate?
>> That would seem to cover 99% of all 32-bit risc machines.
> 
> I was asking for the generic expander to consider bswapHI.  Rotates are
> certainly more likely to get combined with sth else.

Maybe.  Alternately, don't we canonicalize byte-swapping memory ops as

 (set (reg:HI) (bswap:HI (mem:HI)))

 (set (mem:HI) (bswap:HI (reg:HI)))

Certainly this is true for powerpc.

(It appears that both sparc and s390 are missing similar patterns,
despite having instructions that can perform this operation...)


r~


r~

RE: [PATCH] Don't check for optab for 16bit bswap

[Thomas Preud'homme]

> From: Richard Henderson [mailto:rth@redhat.com]
> Sent: Friday, January 23, 2015 2:43 AM
> On 01/21/2015 11:52 PM, Richard Biener wrote:
> >
> > I was asking for the generic expander to consider bswapHI.  Rotates are
> > certainly more likely to get combined with sth else.
> 
> Maybe.  Alternately, don't we canonicalize byte-swapping memory ops
> as
> 
>  (set (reg:HI) (bswap:HI (mem:HI)))
> 
>  (set (mem:HI) (bswap:HI (reg:HI)))

Given the expmed.c bit I pasted, I'd say that's how it would be canonicalized
in RTL form yes. I think Richard was referring to combination at GIMPLE level
but that expander should expand to bswapHI to not break expectations.

Best regards,

Thomas 

Re: [PATCH] Don't check for optab for 16bit bswap

[Richard Biener]

On Thu, 22 Jan 2015, Richard Henderson wrote:

> On 01/21/2015 11:52 PM, Richard Biener wrote:
> > On January 21, 2015 10:23:56 PM CET, Richard Henderson <rth@redhat.com> wrote:
> >> On 12/29/2014 06:04 AM, Thomas Preud'homme wrote:
> >>> Since 16bit byteswap can be done via an 8 bit rotation (and it is the
> >> canonical form),
> >>> the check for an optab only serves to prevent the bswap optimization
> >> for
> >>> targets that don't have a 16bit byteswap (but do have a rotation
> >> instruction). See
> >>> PR63259 (comments 6 and onwards) for more details.
> >>
> >> I question the choice to have rotate be the canonical form.
> >>
> >> Doesn't this make things more complicated for targets that
> >> don't have rotate?  Or, equivalently, no 16-bit rotate?
> >> That would seem to cover 99% of all 32-bit risc machines.
> > 
> > I was asking for the generic expander to consider bswapHI.  Rotates are
> > certainly more likely to get combined with sth else.

Sorry for being GIMPLE centric here - but I meant canonicalization
on GIMPLE.  For RTL the expander will hopefully canonicalize it to
whatever is canonical on RTL.

> Maybe.  Alternately, don't we canonicalize byte-swapping memory ops as
> 
>  (set (reg:HI) (bswap:HI (mem:HI)))
> 
>  (set (mem:HI) (bswap:HI (reg:HI)))
> 
> Certainly this is true for powerpc.
> 
> (It appears that both sparc and s390 are missing similar patterns,
> despite having instructions that can perform this operation...)

That makes sense, it looks easier to match than a form with rotates.
I wonder when CPUs start to make a load-with-byte-shuffle instruction
available...

Richard.